Download it now!
Bug 94557 - Combo box entries are case-sensitive (sometimes)
Summary: Combo box entries are case-sensitive (sometimes)
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: UI (show other bugs)
Version:
(earliest affected)
4.4.5.2 release
Hardware: Other All
: medium minor
Assignee: Not Assigned
URL:
Whiteboard: target:5.1.0 target:4.4.7
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2015-09-28 04:29 UTC by Andreas Säger
Modified: 2016-10-25 19:17 UTC (History)
9 users (show)

See Also:
Crash report or crash signature:


Attachments
Database with form and 2 combo boxes (12.87 KB, application/vnd.sun.xml.base)
2015-09-28 04:41 UTC, Andreas Säger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Säger 2015-09-28 04:29:35 UTC
Contrary to the previous and expected behaviour in LO4.3.x, stand-alone combobox form controls are case sensitive. A combobox within a grid control is not.
Comment 1 Andreas Säger 2015-09-28 04:41:01 UTC
Created attachment 119081 [details]
Database with form and 2 combo boxes
Comment 2 Buovjaga 2015-10-01 11:37:36 UTC
Confirmed.

Win 7 Pro 64-bit, Version: 5.0.2.2 (x64)
Build ID: 37b43f919e4de5eeaca9b9755ed688758a8251fe
Locale: fi-FI (fi_FI)
Comment 3 raal 2015-10-08 12:10:26 UTC
This seems to have begun at the below commit.
Adding Cc: to serval2412@yahoo.fr ; Could you possibly take a look at this
one? Thanks

bibisect-win32-5.0:
1ae649f92b4ef23f32b2df910b8e2cbddc50ceec is the first bad commit
commit 1ae649f92b4ef23f32b2df910b8e2cbddc50ceec
Author: Norbert Thiebaud <nthiebaud@gmail.com>
Date:   Thu May 28 10:36:48 2015 -0500

    source sha:76f33f10309b0ee384a75a7a854858b068d60495

    source sha:76f33f10309b0ee384a75a7a854858b068d60495

:040000 040000 d4a93e2fe2af9783aa233daa584994294fd01079 04b4eb794147a8adb4fed969de9305d073dfd4a9 M      instdir

author	Julien Nabet <serval2412@yahoo.fr>	2015-03-16 21:31:23 (GMT)
committer	Caolán McNamara <caolanm@redhat.com>	2015-03-26 14:14:44 (GMT)
commit	76f33f10309b0ee384a75a7a854858b068d60495 (patch)
tdf#67990: Management of case in combobox
Comment 4 Julien Nabet 2015-10-08 14:35:02 UTC
Thank you for your ping, I'll take a look at it.
Comment 5 Julien Nabet 2015-10-08 14:38:22 UTC
Robert/Lionel: I can try to search where is managed combobox within a grid control but I'd like to be sure that we want it to be case sensitive. Any thoughts?
Comment 6 Andreas Säger 2015-10-08 14:49:15 UTC
(In reply to Julien Nabet from comment #5)
> Robert/Lionel: I can try to search where is managed combobox within a grid
> control but I'd like to be sure that we want it to be case sensitive. Any
> thoughts?

No search box in this virtual world is case-sensitive.
Comment 7 Lionel Elie Mamane 2015-10-08 15:43:17 UTC
(In reply to Julien Nabet from comment #5)
> Robert/Lionel: I can try to search where is managed combobox within a grid
> control but I'd like to be sure that we want it to be case sensitive. Any
> thoughts?

<sigh> Somebody entering a value in the list certainly expects that it is found case-insensitively. OTOH, I understand the concern of the reporter of bug 67990, that he also wants to be able to force two different values with differing case.

I think of this bug and bug 67990, this one is the worst one, and the immediate action should be to revert your previous commit. If we can fix bug 67990 in a way that does not reintroduce this one (I'm not sure how yet), then by all means, that's even better.

commit 76f33f10309b0ee384a75a7a854858b068d60495
Author: Julien Nabet <serval2412@yahoo.fr>
Date:   Mon Mar 16 22:31:23 2015 +0100

    tdf#67990: Management of case in combobox


    1) Combobox were used with autocomplete with default (false) value for matchCase
    => so initialize autocomplete with true value for matchCase
    2) FindMatchingEntry uses bLazy as !matchCase
    but when bLazy = false, no autocomplete can work since you must type the whole word
    so just use "entryCombo" startsWith "typed string" instead of "entryCombo" == "typed string"


Point 1: that's probably exactly what introduced this bug.

Point 2: there seems to be some confusion in the code. "bLazy" seems to mean "do exact match only, not autocomplete". Let's see how it is called from vcl/source/control/combobox.cxx, function void ComboBox::Impl::ImplAutocompleteHandler( Edit* pEdit )


        sal_Int32 nPos = LISTBOX_ENTRY_NOTFOUND;
        if (!m_isMatchCase)
        {
            // Try match case insensitive from current position
            nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward );
            if ( nPos == LISTBOX_ENTRY_NOTFOUND )
                // Try match case insensitive, but from start
                nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText,
                    bForward ? 0 : (m_pImplLB->GetEntryList()->GetEntryCount()-1),
                    bForward );
        }

        if ( nPos == LISTBOX_ENTRY_NOTFOUND )
            // Try match full from current position
            nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, false );
        if ( nPos == LISTBOX_ENTRY_NOTFOUND )
            //  Match full, but from start
            nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText,
                bForward ? 0 : (m_pImplLB->GetEntryList()->GetEntryCount()-1),
                bForward, false );



There, bLazy seems to be used to ask for a *full* match, not a prefix match. I' don't understand why, but the comments make that intent clear.
Comment 8 Lionel Elie Mamane 2015-10-08 15:45:43 UTC
See commit 50ce1398b3c767a8ea52bf3d52fae0d57bf4cb5d and https://bz.apache.org/ooo/show_bug.cgi?id=71064
Comment 9 Lionel Elie Mamane 2015-10-08 15:46:43 UTC
(In reply to Lionel Elie Mamane from comment #7)

> There, bLazy seems to be used to ask for a *full* match, not a prefix match.
> I' don't understand why, but the comments make that intent clear.

Possibly the intent is there, but is wrong and that's a latent bug right there :)
Comment 10 Andreas Säger 2015-10-08 16:58:26 UTC
The list box control works case insensitively as well. If you want to please both request, you can do it like Emacs: When the first letter is upper case, the search is case sensitive, case insensitive otherwise.
But then it should work equally with all list/combo boxes in grids and stand-alone, with name pickers (font selector) and stuff.
Comment 11 Julien Nabet 2015-10-08 18:09:39 UTC
Lionel: I understand I should revert partly http://cgit.freedesktop.org/libreoffice/core/commit/?id=76f33f10309b0ee384a75a7a854858b068d60495 to have this:
pComboBox->EnableAutocomplete( n != 0, false );
instead of this:
pComboBox->EnableAutocomplete( n != 0, true );
in both cases.

Then http://opengrok.libreoffice.org/xref/core/vcl/source/control/ilstbox.cxx#263 is obviously wrong:
    263         if ( bLazy  )
    264         {
    265             bMatch = rI18nHelper.MatchString( rStr, pImplEntry->maStr );
    266         }
    267         else
    268         {
    269             bMatch = rStr.isEmpty() || (pImplEntry->maStr.startsWith(rStr));
    270         }
If changing this part, I suppose we should change the default value here:
sal_Int32           FindMatchingEntry( const OUString& rStr, sal_Int32  nStart = 0, bool bForward = true, bool bLazy = true ) const;
http://opengrok.libreoffice.org/xref/core/vcl/inc/ilstbox.hxx#119

There something I don't understand about the method you quoted :
treatment is almost the same for m_isMatchCase = true or false, except in one case bLazy value. But bLazy is not related to m_isMatchCase?
Or perhaps should we add isMatchCase parameter to FindMatchingEntry?

Quite confusing...
Comment 12 Lionel Elie Mamane 2015-10-08 19:52:15 UTC
(In reply to Julien Nabet from comment #11)
> Lionel: I understand I should revert partly
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=76f33f10309b0ee384a75a7a854858b068d60495 to have this:
> pComboBox->EnableAutocomplete( n != 0, false );
> instead of this:
> pComboBox->EnableAutocomplete( n != 0, true );
> in both cases.

Yes, reverting that will I think fix this bug.

The second

> Then
> http://opengrok.libreoffice.org/xref/core/vcl/source/control/ilstbox.cxx#263
> is obviously wrong:
>     263         if ( bLazy  )
>     264         {
>     265             bMatch = rI18nHelper.MatchString( rStr, pImplEntry->maStr );
>     266         }
>     267         else
>     268         {
>     269             bMatch = rStr.isEmpty() || (pImplEntry->maStr.startsWith(rStr));
>     270         }

Please explain what you see as obvious, I'm rather confused at this point.

I'm not sure what bLazy is supposed to mean. It seems to be related to
"match case-sensitive or case insensitive", since rI18nHelper.MatchString does it
case-insensitive, but pImplEntry->maStr.startsWith(rStr) (which is there after your commit)
and (pImplEntry->maStr.startsWith == rStr) (which was there before your patch) do it
case-sensitive.

On the other hand the place that calls with bLazy==false (in ComboBox::ImplAutocompleteHandler)
has in comments "match full" which I understand "match full string from beginning to end,
not prefix (start)". Which matches the implementation that was there _before_ your commit,
which would mean that your commit did "the wrong thing" and should be reverted also there.
*But* if we go down that path, then the code in ComboBox::ImplAutocompleteHandler doesn't
make complete sense to me, since bLazy==true is called "only" when m_isMatchCase==false...
why would case sensitivity be linked to "match full string vs match prefix"?

The code seems to link "full string match vs prefix match" and "case-sensitive vs case-insensitive" and I don't see any reason for that. But I don't know which of the two it is really supposed to be, I see hints in both directions.

> If changing this part, I suppose we should change the default value here:
> sal_Int32           FindMatchingEntry( const OUString& rStr, sal_Int32 
> nStart = 0, bool bForward = true, bool bLazy = true ) const;
> http://opengrok.libreoffice.org/xref/core/vcl/inc/ilstbox.hxx#119

Why?

> There something I don't understand about the method you quoted :

For the sake of making sure there is no misunderstanding between us, the method I quoted being ComboBox::ImplAutocompleteHandler

> treatment is almost the same for m_isMatchCase = true or false,

No, it is not almost the same, in the case "m_isMatchCase == false",
there are two calls to FindMatchingEntry with bLazy==true, which are not
made if m_isMatchCase == true.

> except in one case bLazy value.

Well, precisely that's quite a difference :)

> But bLazy is not related to m_isMatchCase?

That's exactly the quandary... Should it be related or not? Does it mean "Match Case (inverted)" or does it mean something else?

> Or perhaps should we add isMatchCase parameter to FindMatchingEntry?

Maybe we should just rename bLazy to bMatchCaseInsensitive :)
(but then we should probably call it bMatchCase and reverse its meaning
and reverse the value in all callers and default value).
Comment 13 Julien Nabet 2015-10-08 20:00:54 UTC
Argh! I understood bLazy=true as "use startsWith" whereas bLazy=false as "use full match".
Indeed, the parameter name should be changed! :-)
Now, should I revert completely my commit to have a good basis to rework this part?
Comment 14 Lionel Elie Mamane 2015-10-08 20:22:20 UTC
Yes, I found it.

From

commit d7cd7e89e36fc371b6a81d9065b3fa0b65eb822f
Author: Noel Grandin <noel@peralex.com>
Date:   Wed Jul 24 15:39:49 2013 +0200

    convert vcl/ilistbox from XubString to OUString


this change is buggy:

-        bool bMatch = bLazy ? rI18nHelper.MatchString( rStr, pImplEntry->maStr ) != 0 : ( rStr.Match( pImplEntry->maStr )
+        bool bMatch;
+        if ( bLazy  )
+        {
+            bMatch = rI18nHelper.MatchString( rStr, pImplEntry->maStr ) != 0;
+        }
+        else
+        {
+            bMatch = rStr.isEmpty() || (rStr == pImplEntry->maStr );
+        }


In the bLazy == false case, it went from "match prefix case-sensitively" to "match whole string case-sensitively"! That's where this whole "match whole string comes from", it was an error that your commit rightfully corrected.

So now, my opinion is:

1) "bLazy" has a horrible confusing name. Rename it as mentioned in my previous comment.

2) the comments ComboBox::Impl::ImplAutocompleteHandler are such bad English so as to be confusing.
   Change "full match" there to "case-sensitive match"

Let's look at where these comments were introduced:

commit 37c4087daf7aaeceba18f3149c2d320b998a9b54
Author: Malte Timmermann <mt@openoffice.org>
Date:   Tue Oct 9 11:09:05 2001 +0000

    #91553# ComboBox AutoComplete - prefer  case matching entry...


         BOOL bLazy = mbMatchCase ? FALSE : TRUE;
-        USHORT nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, bLazy );
+        // 1. Try match full from current position
+        USHORT nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, FALSE );
         if ( nPos == LISTBOX_ENTRY_NOTFOUND )
+            // 2. Match full, but from start
+            nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, bForward ? 0 : (mpImplLB->GetEntryList()->Get
+        if ( ( nPos == LISTBOX_ENTRY_NOTFOUND ) && bLazy )
+            // 3. Try match lazy from current position
+            nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, TRUE );
+        if ( ( nPos == LISTBOX_ENTRY_NOTFOUND ) && bLazy )
+            // 4. Try match lazy, but from start
             nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, bForward ? 0 : (mpImplLB->GetEntryList()->Get


There, it clearly is about "case sensitive / insensitive". But this "preference" for "case matching entry" got essentially reverted later:

ommit 50ce1398b3c767a8ea52bf3d52fae0d57bf4cb5d
Author: Kurt Zenker <kz@openoffice.org>
Date:   Wed Jun 25 13:29:42 2008 +0000

    INTEGRATION: CWS vcl89 (1.48.16); FILE MERGED
    2008/05/22 13:07:10 pl 1.48.16.2: #i71064# translate german comment
    2008/05/22 13:04:23 pl 1.48.16.1: #i71064# case insensitive autocomplete should be case insensitive
@@ -364,18 +363,23 @@ IMPL_LINK( ComboBox, ImplAutocompleteHdl, Edit*, pEdit )
             bForward = FALSE;
             nStart = nStart ? nStart - 1 : mpImplLB->GetEntryList()->GetEntryCount()-1;
         }
-        BOOL bLazy = mbMatchCase ? FALSE : TRUE;
-        // 1. Try match full from current position
-        USHORT nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, FALSE );
+
+        USHORT nPos = LISTBOX_ENTRY_NOTFOUND;
+        if( ! mbMatchCase )
+        {
+            // Try match case insensitive from current position
+            nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, TRUE );
+            if ( nPos == LISTBOX_ENTRY_NOTFOUND )
+                // Try match case insensitive, but from start
+                nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, bForward ? 0 : (mpImplLB->GetEntryList()-
+        }
+
         if ( nPos == LISTBOX_ENTRY_NOTFOUND )
-            // 2. Match full, but from start
+            // Try match full from current position
+            nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, FALSE );
+        if ( nPos == LISTBOX_ENTRY_NOTFOUND )
+            //  Match full, but from start
             nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, bForward ? 0 : (mpImplLB->GetEntryList()->Get
-        if ( ( nPos == LISTBOX_ENTRY_NOTFOUND ) && bLazy )
-            // 3. Try match lazy from current position
-            nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, TRUE );
-        if ( ( nPos == LISTBOX_ENTRY_NOTFOUND ) && bLazy )
-            // 4. Try match lazy, but from start
-            nPos = mpImplLB->GetEntryList()->FindMatchingEntry( aStartText, bForward ? 0 : (mpImplLB->GetEntryList()->Get
 
         if ( nPos != LISTBOX_ENTRY_NOTFOUND )
         {



Now, the preference for the preference for the "case matching entry" is basically cancelled. Actually, I think we can replace WITHOUT CHANGE OF BEHAVIOUR:

        if (!m_isMatchCase)
         {
             // Try match case insensitive from current position
             nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward );
             if ( nPos == LISTBOX_ENTRY_NOTFOUND )
                 // Try match case insensitive, but from start
                 nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText,
                     bForward ? 0 : (m_pImplLB->GetEntryList()->GetEntryCount()-1),
                     bForward );
         }
       if ( nPos == LISTBOX_ENTRY_NOTFOUND )
             // Try match full from current position
             nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, false );
         if ( nPos == LISTBOX_ENTRY_NOTFOUND )
             //  Match full, but from start
             nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText,
                 bForward ? 0 : (m_pImplLB->GetEntryList()->GetEntryCount()-1),
                 bForward, false );


by


             // Try match from current position
             nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText, nStart, bForward, !m_isMatchCase );
             if ( nPos == LISTBOX_ENTRY_NOTFOUND )
                 // Try match case insensitive, but from start
                 nPos = m_pImplLB->GetEntryList()->FindMatchingEntry( aStartText,
                     bForward ? 0 : (m_pImplLB->GetEntryList()->GetEntryCount()-1),
                     bForward, !m_isMatchCase );

These two calls will make the same work as the FOUR calls before. 'nuff said.


(if we toggle the behaviour of bLazy-renamed-to-bMatchCase, then obviously the "!" before m_isMatchCase should be removed)


Am I making sense? Do you both understand what I mean and agree with it?
Comment 15 Lionel Elie Mamane 2015-10-08 20:26:07 UTC
(In reply to Julien Nabet from comment #13)
> Argh! I understood bLazy=true as "use startsWith" whereas bLazy=false as
> "use full match".

I did, too. That's exactly the confusion. Before your commit, it *did* have the behaviour of "use full match" vs "use prefix match", but that was a bug introduced in 2013 by Noel. Your commit *corrected* it to how it was before, that is *always* do "prefix match".

> Indeed, the parameter name should be changed! :-)
> Now, should I revert completely my commit to have a good basis to rework
> this part?

No, the second part of your commit is good. Revert the first part of your commit.

I think we can also remove the "rStr.isEmpty() || " in the line

            bMatch = rStr.isEmpty() || (pImplEntry->maStr.startsWith(rStr));

because if rStr is empty, then pImplEntry->maStr.startsWith(rStr)) will always return "true" (but please test that; thanks).
Comment 16 Julien Nabet 2015-10-08 20:49:23 UTC
(In reply to Lionel Elie Mamane from comment #15)
....
> No, the second part of your commit is good. Revert the first part of your
> commit.
> 
> I think we can also remove the "rStr.isEmpty() || " in the line
> 
>             bMatch = rStr.isEmpty() || (pImplEntry->maStr.startsWith(rStr));
> 
> because if rStr is empty, then pImplEntry->maStr.startsWith(rStr)) will
> always return "true" (but please test that; thanks).

So I partly reverted my commit and change the line you quoted here:
https://gerrit.libreoffice.org/#/c/19260/

I must recognize, I haven't tested last part but I suppose startsWith with empty string as parameter should indeed return true.
Comment 17 Commit Notification 2015-10-09 06:03:26 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7ca450cbe524807d51893382fcaa0d1a47c939c6

Related tdf#94557: Combo box entries are case-sensitive (sometimes)

It will be available in 5.1.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 18 Commit Notification 2015-10-09 06:47:36 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "libreoffice-4-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=75c3395de205fda16054e70d1dd296ffdc603426&h=libreoffice-4-4

Related tdf#94557: Combo box entries are case-sensitive (sometimes)

It will be available in 4.4.7.

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

Affected users are encouraged to test the fix and report feedback.
Comment 19 Robinson Tryon (qubit) 2015-12-14 04:50:13 UTC Comment hidden (obsolete)
Comment 20 Andreas Säger 2016-02-22 18:40:21 UTC
I think this can be marked as solved.
May I?
Comment 21 Buovjaga 2016-02-22 21:00:08 UTC
(In reply to Andreas Säger from comment #20)
> I think this can be marked as solved.
> May I?

If you are happy with the fixes, sure :) Not seeing any mention from Lionel or Julien about further work, so I guess it is fine.
Comment 22 Julien Nabet 2016-02-22 21:24:25 UTC
(In reply to Andreas Säger from comment #20)
> I think this can be marked as solved.
> May I?
You just did it in the same time you dropped this comment :-)
Anyway, no pb for me.