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

[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).

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