Bug 158314 - Autofilter dropdown list always shows "Empty" and "Error" enties as active
Summary: Autofilter dropdown list always shows "Empty" and "Error" enties as active
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
7.5.0.0 alpha0+
Hardware: All All
: medium normal
Assignee: Kevin Suo
URL:
Whiteboard: target:24.8.0 target:24.2.1 target:7.6.5
Keywords: bisected, regression
Depends on:
Blocks: AutoFilter
  Show dependency treegraph
 
Reported: 2023-11-22 09:17 UTC by Kevin Suo
Modified: 2024-06-07 12:43 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
simple test spreadsheet (9.13 KB, application/vnd.oasis.opendocument.spreadsheet)
2023-11-22 09:17 UTC, Kevin Suo
Details
updated test file with empty and error cells (10.07 KB, application/vnd.oasis.opendocument.spreadsheet)
2023-12-17 11:14 UTC, Kevin Suo
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Suo 2023-11-22 09:17:48 UTC
Created attachment 190961 [details]
simple test spreadsheet

Steps to Reproduce:

1. Assume we have the following in a spreadsheet:
a	b
1	5
2	<blank>
3	7
4	8

(Note that the place marked as <blank> should be left empty)

2. Set autofilter, select "1" in the drop-down of column a, OK, then click the drop-down of column b.

Current Behaviour:
The drop-down of column b shows both "(empty)" and "5" as active.

Expected:
The drop-down of column b shows only "5" as active, because in this case the only remaining record is "5".

Version: 24.2.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 0660a763466bfa0012d16f60f82b0ffb2f341b64
CPU threads: 8; OS: Linux 6.5; UI render: default; VCL: gtk3
Locale: zh-CN (zh_CN.UTF-8); UI: zh-CN
Calc: threaded

also in
Version: 7.6.4.0.0+ (X86_64) / LibreOffice Community
Build ID: bab433911bdecb344f7ea94dbd00690241a08c54
CPU threads: 8; OS: Linux 6.5; UI render: default; VCL: gtk3
Locale: he-IL (zh_CN.UTF-8); UI: zh-CN
Calc: threaded
Comment 1 m_a_riosv 2023-11-22 12:47:51 UTC
Reproducible
Version: 7.6.3.2 (X86_64) / LibreOffice Community
Build ID: 29d686fea9f6705b262d369fede658f824154cc0
CPU threads: 16; OS: Windows 10.0 Build 22631; UI render: default; VCL: win
Locale: es-ES (es_ES); UI: en-US Calc: CL threaded

It is only a presentation issue, because selecting only empty in column 'b' shows no rows.
Comment 2 Rainer Bielefeld Retired 2023-11-22 12:51:00 UTC
For me already REPRODUCIBLE with reporter's sample doc and Server Installation of Version: 7.5.0.0.alpha0+  Build ID: 2a7fcaf582df3ada57ca519b50e29011973a1b6f
CPU-Threads: 12; BS: Windows 10.0 Build 19044; UI render: default; VCL: win
Locale: de-DE (de_DE); UI: en-US
Calc: Calc: threaded |  Elementary Theme  |  Special devUserProfile  (based on my normal one)

NOT yet reproducible with Server Installation of Version: 7.4.0.0.alpha0+ (x64)  Build ID ae36ee4f3aa544e53e2edad93d6d79160b27bc9d
CPU threads: 12; OS: Windows 10.0 Build 19042; UI render: Skia/Raster; VCL: win
Locale: de-DE (de_DE); UI: en-US  |  Calc: CL  |  Colibri Theme  |  Special devUserProfile

I did not do any additional research (differences in User Profile, Preferences, ...

Additional Info:
================
a) I see a change in UI between 7.4 and 7.5: 
a1) Until LibO 7.4:
    After having applied filter on column "A", Autofilter Selector only shows 
    cell contents values left visible in Column "B"
a2) Beginning with 7.5:
    After having applied filter on column "A", Autofilter Selector also shows 
    cell contents values having been hidden in Column "B". But greyed and
    not selectable

And I agree: "empty" is ok in the dropdown, but it must be shown inactive / greyed
Comment 3 Kevin Suo 2023-11-26 12:53:48 UTC
This may have started from the feature which shows the inactive filter entries grayed-out.

This is more than a "presentation issue". Each time when I click an autofilter dropdown I thought that there are empty rows in a column, while actually there is not.
Comment 4 Kevin Suo 2023-11-29 14:51:12 UTC
Adding Balazs.Varga to cc: would you please take a look?
Comment 5 Kevin Suo 2023-12-16 17:51:39 UTC
Reverting commit 2d1df9f3dccc10f13b8585ad18afce1542ebc4d1 (tdf#117276 sc: Show hidden filter elements as inactive elements) and its related commits 2085e90fe8ac129bc4dbac4612d1ea7544335dae (FilteredRow is not a property of the column, tdf#117276 follow-up), 7321db3cadc8c0e4437ca04e5dcb652734ea9c26 (Related tdf#117276 sc: Show hidden filter elements as inactive elements), and 19533948370dc1ccd7334dbe1a8b7fc8330b10c0 (Name FilteredRow what it is, not hidden; tdf#117276 follow-up) resolves the problem.

Mark as regression and bisected.
Comment 6 Balázs Varga (allotropia) 2023-12-16 19:21:36 UTC
(In reply to Kevin Suo from comment #5)
> Reverting commit 2d1df9f3dccc10f13b8585ad18afce1542ebc4d1 (tdf#117276 sc:
> Show hidden filter elements as inactive elements) and its related commits
> 2085e90fe8ac129bc4dbac4612d1ea7544335dae (FilteredRow is not a property of
> the column, tdf#117276 follow-up), 7321db3cadc8c0e4437ca04e5dcb652734ea9c26
> (Related tdf#117276 sc: Show hidden filter elements as inactive elements),
> and 19533948370dc1ccd7334dbe1a8b7fc8330b10c0 (Name FilteredRow what it is,
> not hidden; tdf#117276 follow-up) resolves the problem.
> 
> Mark as regression and bisected.

I will try to look at it soon. Thanks for the notice.
Comment 7 Kevin Suo 2023-12-17 03:44:31 UTC
Balázs Varga: Thanks in advance. Below are some FYI.

The problem should be in:
https://opengrok.libreoffice.org/xref/core/sc/source/core/data/column3.cxx?r=8b236923#2587

        if (bIsEmptyCell)
        {
            if (!mrFilterEntries.mbHasEmpties)
            {
                mrFilterEntries.push_back(ScTypedStrData(OUString()));
                mrFilterEntries.mbHasEmpties = true;
            }
            return;
        }

It uses mrFilterEntries.push_back(ScTypedStrData(OUString())), while the ScTypedStrData() defaults to treat "bIsHiddenByFilter = false", so an empty entry is always "not hidden by filter".

The same bug may exists for Error cells:

                if (nErr != FormulaError::NONE)
                {
                    // Error cell is evaluated as string (for now).
                    OUString aErr = ScGlobal::GetErrorString(nErr);
                    if (!aErr.isEmpty())
                    {
                        mrFilterEntries.push_back(ScTypedStrData(std::move(aErr)));
                        return;
                    }
Comment 8 Kevin Suo 2023-12-17 11:09:24 UTC
There may be another problem you should think of.

Before this feature, "(empty)" is shown at the top of the autofilter dropdown if it is not filterd, and is not shown if it is filtered. See sc/source/ui/view/gridwin.cxx:1044.

With this feature, "(empty)" is still correctly shown at the top if not filtered, but where should you put it in case it is filtered (i.e. inactive)? At the top of all entries? Or at the top of all *inactive* entries? Or, move it to the bottom?
Comment 9 Kevin Suo 2023-12-17 11:11:32 UTC
Below is a patch which works for me. However, I may not submit to gerrit because it's better to include a test case but I am not familiar with the test case code.

diff --git a/sc/source/core/data/column3.cxx b/sc/source/core/data/column3.cxx
index a0b0c639a003..79027226387e 100644
--- a/sc/source/core/data/column3.cxx
+++ b/sc/source/core/data/column3.cxx
@@ -2584,7 +2584,7 @@ class FilterEntriesHandler
         {
             if (!mrFilterEntries.mbHasEmpties)
             {
-                mrFilterEntries.push_back(ScTypedStrData(OUString()));
+                mrFilterEntries.push_back(ScTypedStrData(OUString(), 0.0, 0.0, ScTypedStrData::Standard, false, mbFilteredRow));
                 mrFilterEntries.mbHasEmpties = true;
             }
             return;
@@ -2614,7 +2614,7 @@ class FilterEntriesHandler
                     OUString aErr = ScGlobal::GetErrorString(nErr);
                     if (!aErr.isEmpty())
                     {
-                        mrFilterEntries.push_back(ScTypedStrData(std::move(aErr)));
+                        mrFilterEntries.push_back(ScTypedStrData(std::move(aErr), 0.0, 0.0, ScTypedStrData::Standard, false, mbFilteredRow));
                         return;
                     }
                 }
diff --git a/sc/source/ui/view/gridwin.cxx b/sc/source/ui/view/gridwin.cxx
index 3f4f6b219c67..9e8224adf911 100644
--- a/sc/source/ui/view/gridwin.cxx
+++ b/sc/source/ui/view/gridwin.cxx
@@ -1049,7 +1049,7 @@ void ScGridWindow::LaunchAutoFilterMenu(SCCOL nCol, SCROW nRow)
             bool bSelected = true;
             if (!aSelectedValue.empty() || !aSelectedString.empty())
                 bSelected = aSelectedString.count(aStringVal) > 0;
-            else if (bQueryByNonEmpty)
+            else if (bQueryByNonEmpty or it->IsHiddenByFilter())
                 bSelected = false;
             mpAutoFilterPopup->addMember(aStringVal, aDoubleVal, bSelected, it->IsHiddenByFilter());
             aFilterEntries.maStrData.erase(it);
Comment 10 Kevin Suo 2023-12-17 11:14:37 UTC
Created attachment 191471 [details]
updated test file with empty and error cells

1. Select "1" in column 1.
2. Click dropdown in column 2.

Expected:
There should be an "(empty)" entry (at the top?) and it should be unselected and inactive.
There should also be an error cell at the bottom and it should be unselected and inactive.
Comment 11 Kevin Suo 2024-01-16 10:56:03 UTC
I've submitted a patch in:
https://gerrit.libreoffice.org/c/core/+/162166
Comment 12 Commit Notification 2024-01-18 12:08:57 UTC
Kevin Suo committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/89e032e9c4c51f52680c7d8bacf59ab2a34f2180

tdf#158314: show Empty and Error entries as non-selected and inactive...

It will be available in 24.8.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 13 Commit Notification 2024-01-18 16:21:39 UTC
Kevin Suo committed a patch related to this issue.
It has been pushed to "libreoffice-24-2":

https://git.libreoffice.org/core/commit/f14e942e84942b42956a058e737742f5d19daa96

tdf#158314: show Empty and Error entries as non-selected and inactive...

It will be available in 24.2.1.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 14 Commit Notification 2024-01-19 08:48:18 UTC
Kevin Suo committed a patch related to this issue.
It has been pushed to "libreoffice-7-6":

https://git.libreoffice.org/core/commit/94b9579b25fe60d8c4c34a3379f06068a1fe6167

tdf#158314: show Empty and Error entries as non-selected and inactive...

It will be available in 7.6.5.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.