Feature request: Visual page numbers

Asked by Razi Alavizadeh on 2014-07-24

Definition:
Logical page number: The current page number as its position in whole document
Visual page number: The page number labeled on page (e.g.: usually visual page number of the first page of chapter 1 of a book is 1)

How about adding another spinBox that has an option that user mark the first visual page and then this spinBox show the visual page number of current page and on edit accept visual page numbers?* (for simplicity shows 0, -1, -2, ... for pages before first visual page)

*: Indeed this feature is what I am missing on all other viewers and I found QPDFView when I'm searching through Google for a Qt based PDF and DjVu reader for implementing it by myself.
I open this question for read user's ideas about its GUI part, and of course I will be glad if one of developer implement it :-D

Question information

Language:
English Edit question
Status:
Answered
For:
qpdfview Edit question
Assignee:
No assignee Edit question
Last query:
2014-08-30
Last reply:
2014-09-01
Adam Reichold (adamreichold) said : #1

Hello,

I'd say this is a case of patches welcome then. I'll be happy to provide any assistance on the mailing list [1] if you're going to implement it. (But I will probably not find the spare time to do so ATM.)
Also note that some backens already provide the notion of a logical page number that contrasts the page's index in the list of all pages. So it might not even be necessary to have the user make that selection. A good starting point would probably be to expose this mapping via the DocumentView class and extend the plug-in interface so that it can be provided by the backends. Then one can go about integrating this into the user interface.

Best regards, Adam.

[1] https://launchpad.net/~qpdfview

Razi Alavizadeh (srazi) said : #2

Hello,
> I'll be happy to provide any assistance on the mailing list [1]
I subscribed to it, and when I have enough free time I'll implement thiis feature.

> A good starting point would probably be to expose this mapping via the DocumentView class and extend the plug-in interface so that it can be provided by the backends. Then one can go about integrating this into the user interface.
Thanks for the point.

Best Regards,
Razi

Razi Alavizadeh (srazi) said : #3

Hello Adam
> I'd say this is a case of patches welcome then.
I implemented it in two commits (commits are based on revision 1644):
First commit: Support using labels of pages (PDF) [1]
It detects if document pages have labels (using info of first page) and use them on spinBox automatically.
For test you can use this PDF [2].

Second commit (completes the first commit): Added navigating by visual page numbers.
I added GUI option to context-menu of spinBox, after setting up the visual first page, user can use Roman (i, ii, iii, iv, ...) numerals for access to preamble pages (pages before first visual page) also negative numbers are available (-1, -2, ...) to use instead Roman ones. (e.g.: -1 instead of i, -11 instead of xi)

But I didn't implement storing this new setting per file! I need your help on this, Per file settings are saved into perfilesettings_v2, does we need to create a new version (perfilesettings_v3)?

[1] http://pastebin.ubuntu.com/8187070/
[2] http://s5.picofile.com/file/8137923700/1001_vocabulary_spelling_2ed.pdf.html
[3] http://pastebin.ubuntu.com/8187081/

Best Regards,
Razi.

Adam Reichold (adamreichold) said : #4

Hello Razi,

first of all: Thanks for your contribution!

Am 30.08.2014 um 14:03 schrieb S. Razi Alavizadeh:
> Question #252130 on qpdfview changed:
> https://answers.launchpad.net/qpdfview/+question/252130
>
> Status: Answered => Open
>
> S. Razi Alavizadeh is still having a problem:
> Hello Adam
>> I'd say this is a case of patches welcome then.
> I implemented it in two commits (commits are based on revision 1644):
> First commit: Support using labels of pages (PDF) [1]
> It detects if document pages have labels (using info of first page) and use them on spinBox automatically.
> For test you can use this PDF [2].

I have only looked at the first commit so far, but I think we should try
to sort some general stuff first:

While we could technically work using patches and pastebin, I would
rather prefer to use Launchpad's built-in change submission tool, i.e.
merge requests. Of course, this depends on using Bazaar instead of Git
which I would ask you to use for submitting your work as it will make
review, discussion and merging much simpler for me. (This is not a
technical statement about which VCS is superior or anything. It is just
that this projects uses Bazaar and I currently see no reason to change
this.)

Also, please try to separate unrelated changes from your commits, e.g.
the last two hunks of changes to "pdfmodel.cpp" have nothing to do with
page label implementation and should not be within the patch. (Line 289
is really just a typo, so no need for the #ifndef.) (I will apply both
changes upstream now.)

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.) A more loosely coupled
version could be storing references to two slots as QMetaMethods, e.g.
create a new sub class "MappingSpinBox" along the lines of:

class MappingSpinBox : public SpinBox
{
public:
  MappingSpinBox(const char* valueFromText,
                 const char* textFromValue,
                 QObject* parent)
                 : SpinBox(parent)
  {
    QMetaObject* metaObject = parent->metaObject();

    m_valueFromText =
      metaObject->method(metaObject->indexOfMethod(valueFromText));
    m_textFromValue =
      metaObject->method(metaObject->indexOfMethod(textFromValue));
  }

protected:
  int SpinBox::valueFromText(const QString& text) const
  {
    int value;
    bool ok = m_valueFromText.invoke(parent(), Qt::DirectConnection,
                                     Q_RETURN_ARG(value), Q_ARG(text));

    if(!ok)
    {
      value = text.toInt();
    }

    return value;
  }

  QString SpinBox::textFromValue(int val) const
  {
    /* ... */
  }

private:
  QMetaMethod m_valueFromTextMethod;
  QMetaMethod m_textFromValueMethod;

};

which can the be used like:

m_spinBox = new MappingSpinBox(SLOT(spinBoxValueFromText(QString)),
                               SLOT(spinBoxTextFromValue(int)),
                               this);

(Note that I did not even compile this, it's just a sketch.)

> Second commit (completes the first commit): Added navigating by visual page numbers.
> I added GUI option to context-menu of spinBox, after setting up the visual first page, user can use Roman (i, ii, iii, iv, ...) numerals for access to preamble pages (pages before first visual page) also negative numbers are available (-1, -2, ...) to use instead Roman ones. (e.g.: -1 instead of i, -11 instead of xi)

Sorry, have not really at looked at it in-depth because of the above.
Just two small things I noticed while skimming it: You probably want
"menu" to be a "QScopedPointer" instead of "QPointer" and I don't think
the "QPointer" overhead is necessary for "that". (After all, this will
register a clean-up handler and so on.) Also the extra "std::string"
within "intToRoman" seems unnecessary.

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.)

> But I didn't implement storing this new setting per file! I need your
> help on this, Per file settings are saved into perfilesettings_v2, does
> we need to create a new version (perfilesettings_v3)?

If we promote this to a per-file setting then, yes, we need to bump the
version of that particular table.

> [1] http://pastebin.ubuntu.com/8187070/
> [2] http://s5.picofile.com/file/8137923700/1001_vocabulary_spelling_2ed.pdf.html
> [3] http://pastebin.ubuntu.com/8187081/
>
> Best Regards,
> Razi.

Best regards, Adam.

Adam Reichold (adamreichold) said : #5

Hello again,

Am 30.08.2014 um 14:03 schrieb S. Razi Alavizadeh:
> Question #252130 on qpdfview changed:
> https://answers.launchpad.net/qpdfview/+question/252130
>
> Status: Answered => Open
>
> S. Razi Alavizadeh is still having a problem: Hello Adam
>> I'd say this is a case of patches welcome then.
> I implemented it in two commits (commits are based on revision
> 1644): First commit: Support using labels of pages (PDF) [1] It
> detects if document pages have labels (using info of first page)
> and use them on spinBox automatically. For test you can use this
> PDF [2].

Another point is that it should be possible to disable the usage of
page labels since documents may contain useless ones or the user just
prefers plain numbers.

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) and maybe only
adjust the spin box suffix if the label is not just the numeric index.
(Which could go into a helper method in MainWindow which is called
from both the numberOfPagesChanged and currentPageChanged handlers.)
(Performance impacts from looking up page labels could be locked
behind the above setting.)

Minor stuff would be that I'd prefer helper methods that are only used
once to the put in the anonymous namespace in the source file. (It can
always be made visible to other modules later on.) And it seems
inconsistent that the "Jump to page..." would not work using page
labels when the spin box does.

Best regards, Adam.

> Second commit (completes the first commit): Added navigating by
> visual page numbers. I added GUI option to context-menu of spinBox,
> after setting up the visual first page, user can use Roman (i, ii,
> iii, iv, ...) numerals for access to preamble pages (pages before
> first visual page) also negative numbers are available (-1, -2,
> ...) to use instead Roman ones. (e.g.: -1 instead of i, -11 instead
> of xi)
>
> But I didn't implement storing this new setting per file! I need
> your help on this, Per file settings are saved into
> perfilesettings_v2, does we need to create a new version
> (perfilesettings_v3)?
>
> [1] http://pastebin.ubuntu.com/8187070/ [2]
> http://s5.picofile.com/file/8137923700/1001_vocabulary_spelling_2ed.pdf.html
>
>
[3] http://pastebin.ubuntu.com/8187081/
>
> Best Regards, Razi.
>

Razi Alavizadeh (srazi) said : #6

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.

> 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?

> 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?

Best Regards,
Razi

Adam Reichold (adamreichold) said : #7

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.

Razi Alavizadeh (srazi) said : #8

> 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...)
OK, I hope your code-snippet be close to solution because I didn't use QMetaMethod before, I'll ask you if i had problem with using it :-)

Adam Reichold (adamreichold) said : #9

Hello again,

Am 30.08.2014 um 19:31 schrieb S. Razi Alavizadeh:
> Question #252130 on qpdfview changed:
> https://answers.launchpad.net/qpdfview/+question/252130
>
> S. Razi Alavizadeh posted a new comment:
>> It would solve the circular dependency but still not produce a geneic
> 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...)
> OK, I hope your code-snippet be close to solution because I didn't use QMetaMethod before, I'll ask you if i had problem with using it :-)
>

attached is a diff of mapping spin box that compiles cleanly and
*should* work as intended. (I changed the order of constructor
parameters so that Qt Creators auto-completion for the "SLOT" macro
applies.)

Best regards, Adam.

Adam Reichold (adamreichold) said : #10

Ok, of course, I forget, you either need to overwrite "validate" or
apply the same meta-method medicine to it so that MainWindow can
implement it. Sorry.

Am 30.08.2014 um 20:02 schrieb Adam Reichold:
> 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 19:31 schrieb S. Razi Alavizadeh:
>> Question #252130 on qpdfview changed:
>> https://answers.launchpad.net/qpdfview/+question/252130
>>
>> S. Razi Alavizadeh posted a new comment:
>>> It would solve the circular dependency but still not produce a geneic
>> 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...)
>> OK, I hope your code-snippet be close to solution because I didn't use QMetaMethod before, I'll ask you if i had problem with using it :-)
>>
>
> attached is a diff of mapping spin box that compiles cleanly and
> *should* work as intended. (I changed the order of constructor
> parameters so that Qt Creators auto-completion for the "SLOT" macro
> applies.)
>
> Best regards, Adam.
>

Razi Alavizadeh (srazi) said : #11

Hi
> attached is a diff of mapping spin box that compiles cleanly and
*should* work as intended. (I changed the order of constructor
parameters so that Qt Creators auto-completion for the "SLOT" macro
applies.)
Thank you very much :-)
But unfortunately I can't see attached file neither here nor on my gmail.

Best Regards,
Razi.

Adam Reichold (adamreichold) said : #12

Damn, Launchpad questions do not seem to support file attachments.
I'll inline the patch but I am sure Thunderbird will mess up the
formatting...

Am 30.08.2014 um 21:46 schrieb S. Razi Alavizadeh:
> Question #252130 on qpdfview changed:
> https://answers.launchpad.net/qpdfview/+question/252130
>
> S. Razi Alavizadeh posted a new comment: Hi
>> attached is a diff of mapping spin box that compiles cleanly and
> *should* work as intended. (I changed the order of constructor
> parameters so that Qt Creators auto-completion for the "SLOT"
> macro applies.) Thank you very much :-) But unfortunately I can't
> see attached file neither here nor on my gmail.
>
> Best Regards, Razi.
>

=== modified file 'sources/miscellaneous.cpp'
--- sources/miscellaneous.cpp 2014-08-30 14:13:42 +0000
+++ sources/miscellaneous.cpp 2014-08-30 17:57:45 +0000
@@ -297,6 +297,42 @@
     }
 }

+MappingSpinBox::MappingSpinBox(QWidget* parent, const char*
textFromValue, const char* valueFromText) : SpinBox(parent)
+{
+ const QMetaObject* metaObject = parent->metaObject();
+
+ m_textFromValue =
metaObject->method(metaObject->indexOfMethod(textFromValue));
+ m_valueFromText =
metaObject->method(metaObject->indexOfMethod(valueFromText));
+}
+
+QString MappingSpinBox::textFromValue(int val) const
+{
+ QString text;
+ const bool ok = m_textFromValue.invoke(parent(),
Qt::DirectConnection,
+ Q_RETURN_ARG(QString,
text), Q_ARG(int, val));
+
+ if(!ok)
+ {
+ text = SpinBox::textFromValue(val);
+ }
+
+ return text;
+}
+
+int MappingSpinBox::valueFromText(const QString& text) const
+{
+ int val;
+ const bool ok = m_valueFromText.invoke(parent(),
Qt::DirectConnection,
+ Q_RETURN_ARG(int, val),
Q_ARG(QString, text));
+
+ if(!ok)
+ {
+ val = SpinBox::valueFromText(text);
+ }
+
+ return val;
+}
+
 ProgressLineEdit::ProgressLineEdit(QWidget* parent) : QLineEdit(parent),
     m_progress(0)
 {
@@ -337,8 +373,6 @@
     }
 }

-// search line edit
-
 SearchLineEdit::SearchLineEdit(QWidget* parent) :
ProgressLineEdit(parent)
 {
     m_timer = new QTimer(this);

=== modified file 'sources/miscellaneous.h'
--- sources/miscellaneous.h 2014-03-29 10:20:32 +0000
+++ sources/miscellaneous.h 2014-08-30 17:57:35 +0000
@@ -26,6 +26,7 @@
 #include <QComboBox>
 #include <QGraphicsEffect>
 #include <QLineEdit>
+#include <QMetaMethod>
 #include <QPainter>
 #include <QSpinBox>
 #include <QTreeView>
@@ -191,6 +192,25 @@

 };

+// mapping spin box
+
+class MappingSpinBox : public SpinBox
+{
+ Q_OBJECT
+
+public:
+ MappingSpinBox(QObject* parent, const char* textFromValue, const
char* valueFromText);
+
+protected:
+ QString textFromValue(int val) const;
+ int valueFromText(const QString& text) const;
+
+private:
+ QMetaMethod m_textFromValue;
+ QMetaMethod m_valueFromText;
+
+};
+
 // progress line edit

 class ProgressLineEdit : public QLineEdit

Adam Reichold (adamreichold) said : #13

Hello again,

Am 30.08.2014 um 21:46 schrieb S. Razi Alavizadeh:
> Question #252130 on qpdfview changed:
> https://answers.launchpad.net/qpdfview/+question/252130
>
> S. Razi Alavizadeh posted a new comment: Hi
>> attached is a diff of mapping spin box that compiles cleanly and
> *should* work as intended. (I changed the order of constructor
> parameters so that Qt Creators auto-completion for the "SLOT"
> macro applies.) Thank you very much :-) But unfortunately I can't
> see attached file neither here nor on my gmail.
>
> Best Regards, Razi.
>

Since I had some time on my hands today but will probably find no more
time for this in the coming week, I took the liberty to implement the
support for custom page label within trunk based on your first patch
and our discussion here.

As it turned out, the mapping spin box was not yet complete. :-\
Nevertheless, this hopefully provides a sound base to implement
manually setting the first visual page. Even so, there are still some
paper cuts: The jump-to-page and add-bookmark dialogs (and probably a
few other places) will currently not make use of the page labels even
if configured and available.

Best regards, Adam.

Razi Alavizadeh (srazi) said : #14

Hi
> Since I had some time on my hands today but will probably find no more
time for this in the coming week, I took the liberty to implement the
support for custom page label within trunk based on your first patch
and our discussion here.
Thanks, I also implemented them today.

> As it turned out, the mapping spin box was not yet complete. :-\
Nevertheless, this hopefully provides a sound base to implement
manually setting the first visual page. Even so, there are still some
paper cuts: The jump-to-page and add-bookmark dialogs (and probably a
few other places) will currently not make use of the page labels even
if configured and available.
After I update my patch with new revisions and implement jump-to-page and add-bookmark dialogs with SpinBox, I'll send a pull request.

Best Regards,
Razi.

Razi Alavizadeh (srazi) said : #15

Hi Adam,
I push some commits here [1] and I can see it here [2]. Does this mean a pull request in bazaar+launchpad dictionary?

[1] https://code.launchpad.net/~srazi/qpdfview/visual-page-numbering
[2] https://code.launchpad.net/qpdfview

Best Regards,
Razi

Razi Alavizadeh (srazi) said : #16

Oh sorry... there's white space collision.
I'll push them again, ASAP.

Adam Reichold (adamreichold) said : #17

Hello again,

Am 01.09.2014 um 20:51 schrieb S. Razi Alavizadeh:
> Question #252130 on qpdfview changed:
> https://answers.launchpad.net/qpdfview/+question/252130
>
> S. Razi Alavizadeh posted a new comment:
> Oh sorry... there's white space collision.
> I'll push them again, ASAP.

That's a lot of change... ;-)

Concerning the merge request, on [1] you should find a link called
"propose for merging" where you can propose to merge this branch into
e.g. "lp:qpdfview" which is qpdfview's trunk. This will create a proper
merge request via an attached review. Most importantly this will yield a
place where one can follow the diff to the upstream branch as it evolves
and discuss the changes. (So that we do not have to continue blowing up
this question thread.)

Thanks again for your contribution and looking forward to the merge request.

Best regards, Adam.

[1] https://code.launchpad.net/~srazi/qpdfview/visual-page-numbering

Can you help with this problem?

Provide an answer of your own, or ask Razi Alavizadeh for more information if necessary.

To post a message you must log in.