Hello Razi Am 25.09.2014 um 21:35 schrieb Seyyed Razi Alavizadeh: > > > 2014-09-24 23:03 GMT+03:30 Adam Reichold > <[email protected] <mailto:[email protected]>>: > > 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 >> <[email protected] <mailto:[email protected]> >> <mailto:[email protected] > <mailto:[email protected]>>>: > >> 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> > > > -- Mailing list: https://launchpad.net/~qpdfview Post to : [email protected] Unsubscribe : https://launchpad.net/~qpdfview More help : https://help.launchpad.net/ListHelp
