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

Sun Jan 11 13:39:45 PST 2015

El Diumenge, 11 de gener de 2015, a les 22:30:43, Adam Reichold va escriure:
> 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.

That looks good to me, let's give people a week so someone else can comment.

Cheers,
  Albert

> 
> 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