qpdfview team mailing list archive

[Adam Reichold <question252130@xxxxxxxxxxxxxxxxxxxxx>]

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

    Status: Open => Answered

Adam Reichold proposed the following answer:
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.

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