qpdfview team mailing list archive

[Adam Reichold <adam.reichold@xxxxxxxxxxx>]

Hello Razi

Am 25.09.2014 um 21:35 schrieb Seyyed Razi Alavizadeh:
> 
> 
> 2014-09-24 23:03 GMT+03:30 Adam Reichold
> <adam.reichold@xxxxxxxxxxx <mailto:adam.reichold@xxxxxxxxxxx>>:
> 
> 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?

What I mean by on-demand is to not store the accompanying text in the
model at all but rather compute it whenever a view requests that
information from the model, so that only people using the search dock
would suffer from the extra calls to "Page::text". This would also
remove the need for the new result type and remove the layering
violation of detecting lines within Page::search. (Also fixing the
regression is not really possible this way since each call to
"Poppler::Page::text" will produce exactly the overhead that the new
"Page::search" overload is supposed to avoid, i.e. create a
"TextOutputDev" instance for each occurence.)

Best regards, Adam.

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