[poppler] RFC: Expose whole-words search option in Qt frontends

Sun Jan 11 13:30:43 PST 2015

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hello again,

Am 11.01.2015 um 22:13 schrieb Albert Astals Cid:
> El Diumenge, 11 de gener de 2015, a les 22:04:53, Adam Reichold va
> escriure:
>> Hello,
>> 
>> Am 11.01.2015 um 21:50 schrieb Albert Astals Cid:
>>> El Diumenge, 11 de gener de 2015, a les 20:36:44, Adam Reichold
>>> va
>>> 
>>> escriure:
>>>> Hello,
>>>> 
>>>> Am 11.01.2015 um 20:19 schrieb Albert Astals Cid:
>>>>> El Diumenge, 11 de gener de 2015, a les 11:14:12, Adam
>>>>> Reichold va
>>>>> 
>>>>> escriure:
>>>>>> Hello again,
>>>>>> 
>>>>>> Am 11.01.2015 um 00:57 schrieb Albert Astals Cid:
>>>>>>> El Diumenge, 11 de gener de 2015, a les 00:37:36, Adam 
>>>>>>> Reichold va
>>>>>>> 
>>>>>>> escriure:
>>>>>>>> Hello,
>>>>>>>> 
>>>>>>>> Am 10.01.2015 um 23:45 schrieb Albert Astals Cid:
>>>>>>>>> Maybe you can add some tests to check_search.cpp?
>>>>>>>> 
>>>>>>>> Attached patch with a test case for the four flag 
>>>>>>>> combinations added to the Qt4 and Qt5 versions of 
>>>>>>>> "check_search.cpp".
>>>>>>>> 
>>>>>>>> Of course, this very much highlights the ugliness of 
>>>>>>>> shoe horning the flags into the existing enumeration.
>>>>>>>> But I don't think that making "Poppler::Page::search"
>>>>>>>> take an "int" instead of "SearchMode" would be ABI
>>>>>>>> compatible strictly speaking, since now the compiler
>>>>>>>> is free to choose the underlying type for the
>>>>>>>> enumeration.
>>>>>>> 
>>>>>>> Meh, you're right, this is not good either since
>>>>>>> you're passing in SearchMode something that is not a
>>>>>>> SearchMode (it's the sum of two).
>>>>>>> 
>>>>>>> So yeah it'd be better to go the QFlags way. Maybe you
>>>>>>> can leave the old function there, mark it as deprecated
>>>>>>> and say that it only obeys the sensitiviness part and
>>>>>>> add a new function that accepts the qflags?
>>>>>> 
>>>>>> Ok, attached is the patch which adds a new flags type 
>>>>>> SearchFlags and corresponding overloads to the Qt4
>>>>>> frontend. I try to factor out as much common code as
>>>>>> possible and add a test case as well. I'll add the Qt5
>>>>>> frontend as soon we think the approach is sound.
>>>>>> 
>>>>>> I chose to use a new enumeration SearchFlag instead of 
>>>>>> SearchMode since the symmetry of SearchMode does not
>>>>>> really make sense if there are several non-exclusive
>>>>>> flags.
>>>>> 
>>>>> Yep, now thinking out loud, would it make sense to merge 
>>>>> SearchDirection in there too? The three enums are also 
>>>>> exclusive one another.
>>>> 
>>>> I don't think this would work, since the range of 
>>>> "SearchDirection" has more than two elements and hence would
>>>> need at least two flags but then those would not be
>>>> exclusive. E.g. I'd add two flags "NextResult" and
>>>> "PreviousResult" with "FromTop" as the default, but then the
>>>> user could specify "NextResult | PreviousResult" which does
>>>> not make sense?
>>> 
>>> Ok, let's leave it as you suggested, bring back the tests
>>> (adding uses for both deprecated and non deprecated versions)
>>> and the qt5 side and it looks good to me.
>> 
>> Ok, can you be more specific about which additional test cases
>> you'd like to have? (The ones from the previous design are
>> included already.) I'll then add them during the week.
>> ("WholeWords" won't work with the deprecated methods, but I could
>> add test cases for case sensitivity alone and also the
>> list-returning variants.)
> 
> I'd like to have the old tests but using the new non deprecated
> calls (not sure if that was added in the email that crossed the
> intewebs with my other email).

Sorry, that took me a minute to get... :-) Attached is the patch with
the search tests adjusted to use non-deprecated methods.

Best regards, Adam.

> Cheers, Albert
> 
>> 
>> Best regards, Adam.
>> 
>>> Cheers, Albert
>>> 
>>>> Best regards, Adam.
>>>> 
>>>>> But on the other hand it may feel a bit shoehorned.
>>>>> 
>>>>> What do you think?
>>>>> 
>>>>> Cheer,s Albert
>>>>> 
>>>>>> Best regards, Adam.
>>>>>> 
>>>>>>> Cheers, Albert
>>>>>>> 
>>>>>>>> Best regards, Adam.
>>>>>>>> 
>>>>>>>>> Cheers, Albert
>>>>>>>>> 
>>>>>>>>> El Dissabte, 10 de gener de 2015, a les 23:26:01,
>>>>>>>>> Adam Reichold va
>>>>>>>>> 
>>>>>>>>> escriure:
>>>>>>>>>> Hello again,
>>>>>>>>>> 
>>>>>>>>>> Am 10.01.2015 um 22:25 schrieb Albert Astals
>>>>>>>>>> Cid:
>>>>>>>>>>> El Dissabte, 10 de gener de 2015, a les
>>>>>>>>>>> 17:51:48, Adam Reichold va
>>>>>>>>>>> 
>>>>>>>>>>> escriure:
>>>>>>>>>>>> Hello,
>>>>>>>>>>>> 
>>>>>>>>>>>> Attach is a patch that would expose
>>>>>>>>>>>> Poppler's whole-words search option within
>>>>>>>>>>>> the Qt frontends.
>>>>>>>>>>>> 
>>>>>>>>>>>> While the implementation seems straight
>>>>>>>>>>>> forward so far, I would like to request
>>>>>>>>>>>> comments whether extending the "SearchMode"
>>>>>>>>>>>> enumeration to flag definition is considered
>>>>>>>>>>>> harmless. The proper way to do it seems to be
>>>>>>>>>>>> the introduction of "QFlags<SearchMode>"
>>>>>>>>>>>> which would however break compatibility and
>>>>>>>>>>>> hence imply the need to have an additional
>>>>>>>>>>>> overloads using a separate flag (and
>>>>>>>>>>>> enumeration?) definition.
>>>>>>>>>>> 
>>>>>>>>>>> Looks good to me. The qt5/ side would need the 
>>>>>>>>>>> documentation update too, no?
>>>>>>>>>> 
>>>>>>>>>> patch with fixed Qt5 documentation and "\since" 
>>>>>>>>>> commands attached.
>>>>>>>>>> 
>>>>>>>>>> Best regards, Adam.
>>>>>>>>>> 
>>>>>>>>>>> Cheers, Albert
>>>>>>>>>>> 
>>>>>>>>>>>> Best regards, Adam.
>>>>>>>>>>> 
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>
>>>>>>>>>>> 
poppler mailing list poppler at lists.freedesktop.org
>>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>
>>
>>>>>>>>>>> 
_______________________________________________ poppler
>> 
>>>>>>>>> mailing list poppler at lists.freedesktop.org 
>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>
>>>>>>>
>>>>>>>>> 
_______________________________________________ poppler
>>>>>>> mailing list poppler at lists.freedesktop.org 
>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>> 
>>>>> _______________________________________________ poppler 
>>>>> mailing list poppler at lists.freedesktop.org 
>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>> 
>>>> _______________________________________________ poppler
>>>> mailing list poppler at lists.freedesktop.org 
>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>> 
>>> _______________________________________________ poppler
>>> mailing list poppler at lists.freedesktop.org 
>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>> 
>> _______________________________________________ poppler mailing
>> list poppler at lists.freedesktop.org 
>> http://lists.freedesktop.org/mailman/listinfo/poppler
> 
> _______________________________________________ poppler mailing
> list poppler at lists.freedesktop.org 
> http://lists.freedesktop.org/mailman/listinfo/poppler
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJUsut/AAoJEPSSjE3STU34E4cIALJKcQuJxK3CVsfwIMDAxniZ
dpDiSn2Fh1YG0CoBRFfrjRosmv9neUqtSOLurJM93zO463+otXSuMMyUI3QCCeZJ
JBvUG1c2ky4DujCfnQloW27EUi2UPhzM47bWd1UHQwU9QBLCPgcknsneoO/YeYX6
022o4Dq+q4gtq6S43grUJVb67OC5oI33M9TGlMrWh7eDAIrEKXN1t/+zfNaT7e8G
L211ccNHvO7/apVDXIsiCObSpdDn8u4R84f2O47PCPTCkyYXMnLd/bLl8hBt0lS4
P7nMqmyZ4y5TLgvTXwL1XhvCpISja3UixJu9r70vhRV5UB3E9nd45G63O3CkSFc=
=In65
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: whole-words-proper.diff
Type: text/x-patch
Size: 44530 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20150111/a1ffe0ad/attachment-0001.bin>