qpdfview team mailing list archive

[Adam Reichold <question252130@xxxxxxxxxxxxxxxxxxxxx>]

Question #252130 on qpdfview changed:
https://answers.launchpad.net/qpdfview/+question/252130

Adam Reichold proposed the following answer:
Hello again,

Am 30.08.2014 um 18:47 schrieb S. Razi Alavizadeh:
> Question #252130 on qpdfview changed:
> https://answers.launchpad.net/qpdfview/+question/252130
> 
> S. Razi Alavizadeh posted a new comment:
> Hi,
> 
>> first of all: Thanks for your contribution!
> - My pleasure :-)
> 
>> While we could technically work using patches and pastebin, I would rather prefer to use Launchpad's built-in change submission tool ....
> - OK after applying changes that you mentioned above I'll do a pull request. Indeed my problem is with slow and unstable bazaar's client for Windows. Maybe I should try to use and learn its commandline options and commands.

We don't have a lot of Windows exposure, so the tool chain is certainly
biased towards Linux. But I think the Bazaar command line client is
pretty straight-forward to use. I'd be happy to help if I can. (Btw.,
the client is slow (yet stable) everywhere else too. ;-) As I said,
using Bazaar is not about technical superiority, but with this project
about integration into Launchpad.)

>> Also, please try to separate unrelated changes from your commits...
> - Certainly
> 
>> Now for the patches content, I dislike the fact that SpinBox and
> MainWindow get a circular dependency by it (and the coupling seems to
> get increased even more by the second patch.)
> - What about easily add a subclass of SpinBox within MainWindow class? dislike it also?

It would solve the circular dependency but still not produce a generic
component. Why not use the QMetaMethod-based generic class which would
allow the exact same division of labor without close coupling? (How I
would love for this to be a C++11 code base where a lambda stored as a
std::function would solve this beautifully. But we have to build on RHEL
6...)

>> Also just adding an action to the document view context menu (as defined
> by MainWindow::on_currentTab_customContextMenuRequested) might be a
> simpler solution to setting the visual first page. (Copy-pasting Qt code
> to extend widget functionality is a code smell IMHO.)
> - I agree, I also didn't like the hack for adding action to context-menu :-D
> 
> 
>> I am also not really happy with heuristic encoded in
> DocumentView::hasLabelledPages. I would prefer a solution where we
> always have a valid page label (even if it was provided by
> DocumentView if the backend return an empty string)
> 
> - I'm not sure how to handle some special cases, for example:
> On a document some pages have non-empty labels and some have [correctly] empty labels and in another document all pages have empty labels what Page::label() should returns? it seems we have to check all page if they have empty label or not and for big document this seems to me a slow task, I'm wrong?

Personally, I would not consider an empty page label valid and prefer to
replace it by the page number in any case. Also the current heuristic
would not handle all such situations correctly as well, e.g. what if the
first page has an empty label and others do not.

I think "return the backend label if it is non-empty and the page number
as a string otherwise" is a justifiable and most importantly simple
solution. (And "use extended spin box suffix only if the page label is
not the page number as a string" would be a logical extension of it.
(And I do think it would be alright if this changed on a per-page basis.)

> Best Regards,
> Razi
> 

Best regards, Adam.

-- 
You received this question notification because you are a member of
qpdfview, which is an answer contact for qpdfview.