qpdfview

Merge lp://staging/~srazi/qpdfview/some-patches into lp://staging/qpdfview

some-patches
Merge into trunk

Proposed by Razi Alavizadeh on 2018-05-11

Status:	Needs review
Proposed branch:	lp://staging/~srazi/qpdfview/some-patches
Merge into:	lp://staging/qpdfview
Diff against target:	574 lines (+285/-69) 14 files modified application.pro (+2/-0) sources/bookmarkitemdelegate.cpp (+75/-0) sources/bookmarkitemdelegate.h (+45/-0) sources/bookmarkmodel.h (+19/-0) sources/documentview.cpp (+3/-0) sources/documentview.h (+2/-0) sources/mainwindow.cpp (+8/-1) sources/mainwindow.h (+1/-1) sources/miscellaneous.cpp (+114/-0) sources/miscellaneous.h (+3/-0) sources/pageitem.cpp (+6/-0) sources/pageitem.h (+2/-0) sources/searchitemdelegate.cpp (+5/-65) sources/searchitemdelegate.h (+0/-2)
To merge this branch:	bzr merge lp://staging/~srazi/qpdfview/some-patches
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Adam Reichold		2018-05-11	Pending
Review via email: mp+345392@code.staging.launchpad.net

This proposal supersedes a proposal from 2018-05-06.

Description of the change

Refactor of bookmark-work branch and add some other commits.

Revision history for this message

Adam Reichold (adamreichold) wrote on 2018-05-07: Posted in a previous version of this proposal

Hello Razi,

thank you for your contribution! I am happy to be hearing from you again. Also, I will try to review your changes as soon as possible but please be patient since I am currently not sure when I will find the time.

Best regards,
Adam

Revision history for this message

Adam Reichold (adamreichold) wrote on 2018-05-07: Posted in a previous version of this proposal

Hello again,

just two general remarks:

* I think BookmarkDialog::appendComment should become Bookmark::append since it is not involved with the presentation layer at all. (And it is IMHO that only the comment can sensibly be appended to.)

* The text rendering code of BookmarkItemDelegate seems to have quite a bit of duplication with e.g. SearchItemDelegate. Could this be factored out into some common text rendering code? Can we not somehow reuse the existing text rendering used to e.g. draw QLabel?

Best regards,
Adam

Revision history for this message

Razi Alavizadeh (srazi) wrote on 2018-05-07: Posted in a previous version of this proposal

Hello,

> * I think BookmarkDialog::appendComment should become Bookmark::append since it is not involved with the presentation layer at all.

I agree.

> * The text rendering code of BookmarkItemDelegate seems to have quite a bit of duplication with e.g. SearchItemDelegate. Could this be factored out into some common text rendering code? Can we not somehow reuse the existing text rendering used to e.g. draw QLabel?

Yes, indeed it is a modified copy of SearchItemDelegate. Good idea, I will try to refactor it.

Best Regards,
Razi.

Revision history for this message

Razi Alavizadeh (srazi) wrote on 2018-05-11:

Hello again,

This branch also contains some bug fixes:
1- Fix highlighting matchedText when a part of it, is removed by QFontMetrics::elidedText()
2- Fix bookmark tooltip when comment is too long
3- Fix showing search results of a closed tab

Best Regards,
Razi.

Revision history for this message

Adam Reichold (adamreichold) wrote on 2018-05-13:

Hello Razi,

sorry that I do not have to time to review all of this. I did however merge everything that is purely a bug fix into trunk revision 2075. Could you merge trunk back into this branch to make the diff smaller?

From a first look at the code, I noticed that drawAlternativeText should be split into two functions, one with matchedText as they do not share any significant control flow (and common setup could be factored out into a helper function).

Also signal names are usually formulated in the passive voice, i.e. requestAppendToBookmarkComment should rather be appendToBookmarkRequested.

Best regards,
Adam

Revision history for this message

Adam Reichold (adamreichold) on 2018-05-13:

lp://staging/~srazi/qpdfview/some-patches updated on 2018-05-13

2078. By Razi Alavizadeh on 2018-05-13: Merge upstream
2079. By Razi Alavizadeh on 2018-05-13: Improve code style of drawAlternativeText()
2080. By Razi Alavizadeh on 2018-05-13: Rename requestAppendToBookmarkComment() to appendToBookmarkRequested()
2081. By Razi Alavizadeh on 2018-05-13: Fix code style of appendComment() and &

Revision history for this message

Adam Reichold (adamreichold) wrote on 2018-05-28:

I am really sorry that this is taking so long, but I think I will concentrate on making a 0.4.18 for now and revisit this after that, so maybe one or two weeks. Again, I am really sorry to keep you waiting and I am very grateful for your contributions, including this one!

Revision history for this message

Razi Alavizadeh (srazi) wrote on 2018-05-29:

NP :)
Thank you.

Revision history for this message

Adam Reichold (adamreichold) wrote on 2021-04-23:

Hello Razi,

I recently fixed up things so qpdfview will build against Qt versions 4, 5 and 6 and am trying to prepare a 0.5 release. I would also try to reboot this endeavour and include your changes into that version. But after reading it again, I think there are two changes here that would benefit from being discussed separately: Appending selected text to the bookmark comment and an extension of the bookmark view to show comments.

To achieve this, I would suggest the following: I will try to include the append-to-comment part manually (keeping your copyright intact). After that, could you rebase the work on the bookmarks view again onto the current trunk revision? Thank you! (I can only repeat how sorry I am that this laid dormant for such a long time.)

Kind regards,
Adam