qpdfview team mailing list archive

[Seyyed Razi Alavizadeh <s.r.alavizadeh@xxxxxxxxx>]

2014-09-24 23:03 GMT+03:30 Adam Reichold <adam.reichold@xxxxxxxxxxx>:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hello Razi,
>
> Hello Adam,

> thanks for putting so much work into this! I just had a short look at
> the branch and noticed some things:
>
> * It does not build. It fails first in "PdfPage::search" missing
> "m_size" for me. Maybe just "m_page->m_size"?
>
> Oops... it's because I'm using Poppler 0.10 and I just forgot to update
that part of code to the latest code.

* Did you measure the increase in memory consumption because of
> storing the text of the whole line? I suspect it is significant.

 Did you test with a DjVu file? I think maybe what you saw about
memory consumption is because of removing  "Page::search" overload and not
other part of this patch
I used Windows Task Manager for test and for example for ~15000 occurrences
it used ~5MB that I think is as expected.

Also
> extracting this directly inside the model adds UI specifics to it
> which smells like a layering violation. Wouldn't it be better to
> request that text on-demand so that people not using the search dock
> don't pay the additional overhead?
>
>  Yes you are right. The best solution that I'm aware is that we request
text on vertical scrollbar position change.
I wrote a solution (workaround?) for it (revision.1704 [1]) now for me DjVu
search seems as fast as it should be.
By "on-demand" do you mean an option to disable storing result in model,
i.e. disable this patch completely?

* Also not using the "Page::search" overload introduced in Poppler
> 0.22 (funnily enough by us specifically for qpdfview) which returns a
> whole list of occurrences drastically reduces search performance. It
> was up to a factor of 10 if I remember correctly. I do not think users
> will like that and it also joins with the problem stated before.
>
>  Again because I'm using older Poppler I didn't see the regression you
mentioned, but if we request text in other part for example my above
suggestion or in SearchTask::run() we can solve this issue. (revision.1703
[2])

* You are also right that I strongly dislike the kind of reuse that
> "SearchDelegate" embodies. Did you try using a widget delegate to
> achieve the same effect? What exactly are the modifications to
> QItemDelegate? I suppose it is the additional formats highlighting the
> keyword? Is there no way to inject this behavior into an existing
> delegate or widget?
>
Yes I just add additional formats to QTextLayout. I re-wrote it.
(revision.1702 [3])

>
> * I do not like "DocumentView::prepareHighlight" becoming public as it
> is an internal helper method. If anything there could be another
> public slot to jump to a specific search result using some form of
> index where it is ensured that the method is called with proper input.
>
> Revision.1705 [4]


> * If we are going to invest significant resources (code size and
> complexity, memory consumption and processing power) into this, then I
> think we should aim to implement the broader feature, i.e. a search
> interface showing results from all open tabs. Hence placing the
> "SearchModel" outside of the "DocumentView" and connecting to it via
> signals and slots to keep its data current.
>
> I am sorry if this sounds overly critical. It is just that I feel that
> my job description as maintainer is to work the program into the best
> state achievable with the given resources, often meaning to work it
> and its contributors a bit harder to cope with the increasing
> complexity of the project.

So please don't feel discouraged and let's discuss the various issues
> as I am sure we can resolve them. If you want to, you can also reply
> to this mail via the mailing list to make this a public discussion so
> that other people can join in and maybe reweigh some of my objections.
>
> No problem :)  the code of QPDFView is clean and in a very good state
because of you, please keep up your great work.

Best regards, Adam.
>
>
[1] http://bazaar.launchpad.net/~srazi/qpdfview
/extend-search-dock2/revision/1704
[2] http://bazaar.launchpad.net/~srazi/qpdfview
/extend-search-dock2/revision/1703
[3] http://bazaar.launchpad.net/~srazi/qpdfview
/extend-search-dock2/revision/1702
[4] http://bazaar.launchpad.net/~srazi/qpdfview
/extend-search-dock2/revision/1705

Best Regards,
Razi


> Am 24.09.2014 um 14:12 schrieb Seyyed Razi Alavizadeh:
> > Hello Adam, I pushed it here [1], indeed I duplicated and renamed
> > "BookmarkModel" to "SearchModel". And for "SearchDelegate" I know
> > that you don't like copy-paste from Qt codes but I wrote this
> > class more than a year ago and it is tested on two applications, I
> > used QTextLayout because it internally runs bidi-algorithm on text
> > and because of this it works correctly with RTL languages (like my
> > native language: Persian) using QPainter::drawText() is just good
> > when we don't need to support RTL languages.
> >
> > [1] https://code.launchpad.net/~srazi/qpdfview/extend-search-dock2
> >
> > Best Regards, Razi.
> >
> > 2014-09-23 22:18 GMT+03:30 Adam Reichold <adam.reichold@xxxxxxxxxxx
> > <mailto:adam.reichold@xxxxxxxxxxx>>:
> >
> > Hello Razi,
> >
> > you mentioned a branch implementing a search dock once. I justed
> > wanted to suggest to you to publish it on Launchpad so that other
> > people can test it, even though we can't merge it into trunk yet.
> > To get people notified, you could use the mailing list and link
> > your branch to the related bug report [1].
> >
> > Best regards, Adam.
> >
> > [1] https://bugs.launchpad.net/qpdfview7/+bug/1028295
> >
> >
> >
> >
> > -- Alavizadeh, Sayed Razi My Blog: http://pozh.org
> > <http://pozh.org/> Saaghar (نرم‌افزار شعر):
> > http://saaghar.pozh.org/ Saaghar Fan Page:
> > http://www.facebook.com/saaghar.p Saaghar Mailing List:
> > http://groups.google.com/group/saaghar
> >
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQEcBAEBAgAGBQJUIxyRAAoJEPSSjE3STU34/vQIAL2XreiaOGc1hsBORmyisXln
> j6Exq3n8KRpBlUW4vKtFmA90yatty/6btIz0PlTLPJbCBGJcl0qGz5KH3Id/sGKf
> orL/gQFduz8hjp3RKGVbs91q0vLpCim+oxcS/RPQNGxJlZcqJstPRUSVHHJI3fdo
> qM2NoE3hXqt8cqwiIVxhrQeIr7iOzW6IW6TqUrcQk9Y8t9/NsxaGYbgvoUMAfzIk
> 4qI5y0EGudl2TjSK9MnjKX5Sl/nZMPSYiIfMjhZnt6BFq+xZQK9FxdKAjCa4UkCH
> 0B92CDN3fUwPOAuLxEvIuD0x0Gy6QCRT7j1VAB0ntg7ffwPe0oEMj69tSZYAKUU=
> =i7QQ
> -----END PGP SIGNATURE-----
>



-- 
Alavizadeh, Sayed Razi
My Blog: http://pozh.org
Saaghar (نرم‌افزار شعر): http://saaghar.pozh.org/
Saaghar Fan Page: http://www.facebook.com/saaghar.p
Saaghar Mailing List: http://groups.google.com/group/saaghar