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.
Created attachment 119081 [details] Database with form and 2 combo boxes
Confirmed. Win 7 Pro 64-bit, Version: 5.0.2.2 (x64) Build ID: 37b43f919e4de5eeaca9b9755ed688758a8251fe Locale: fi-FI (fi_FI)
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 76f33f10309b0ee384a75a7a854858b068d60495 source 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
Thank you for your ping, I'll take a look at it.
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?
(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.
(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.
See commit 50ce1398b3c767a8ea52bf3d52fae0d57bf4cb5d and https://bz.apache.org/ooo/show_bug.cgi?id=71064
(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 :)
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.
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...
(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).
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?
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?
(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).
(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.
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.
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.
Migrating Whiteboard tags to Keywords: (bibisected) [NinjaEdit]
I think this can be marked as solved. May I?
(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.
(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.