Code review comment for lp://qastaging/~verzegnassi-stefano/ubuntu-docviewer-app/document-hub2

Riccardo, there's also the “debug” policy issue that you spotted this morning.
Dunno why it's there, but I shall fix it before the branch is merged.

Sorry if I reply to the inline comments here, but it's for my brain sanity when I will fix them. :-)

> Line 734: QString filenameWithoutSuffix = filename.left(filename.size() - suffix.size());
It comes from gallery-app

> Line 740: filenameWithoutSuffix += ".";
Same as above.

> Line 757:
My fault here.

> Line 806: \brief ContentCommunicator::returnPhoto returns the given photos
Yes, it is. Forgot to change it (I did it the first time, but I lost the code) :P

> Line 849: return m_transfer->selectionType() == Transfer::SelectionType::single;
TBH dunno, it's the same in the upstream code (gallery-app). I need to check.

> Line 983: bool ok = m_cmdLineParser->processArguments(arguments());
Nice question! Why I did it? Need to fix! :-)

> Line 1069: m_view->setTitle("Document Viewer");
Sure! (Even if the string is replaced by QML Page title)

> Line 1706: QcoreApplication::setOrganizationDomain("com.ubuntu.docviewer");
Huh, I think I choose the wrong hint from QtCreator. :P

> Line 3558:
Yes, I agree with you! This dialog is part of the earlier code of the docviewer (before I adopted the project).
I didn't spent much time on it, but surely it's something I'd like to improve!

> Line 3690:
You're right!

> Line 3695:
As I told to Filippo some time ago, I followed the code style guidelines that has been chosen for the Core Apps project.
Probably it's too much orthodox as implementation.

> Line 3753: viewLoader.item.endSelection();
You're right again!

>Line 3783: not sure which line do you mean?
>> If (import "../upstreamComponents”)
The EmptyState item lives in ../upstreamComponents
>> If you mean the Item that contains the EmptyState, it's because of the alignment inside a QML Loader.

> Line 3837, 3840, 3843, 3845:
Again, you're right about translations!

> Line 3936:
Oops, I think there's another label earlier and I forgot to remove the RowLayout.

> Line 3937, 4107:
I will fix it!

> Line 4110:
Haha

> Line 4325:
Yep! Will fix it!

> Line 4442:
No reason!

> Line 4510:
Nope! As in gallery-app, the button is not enabled but still visible, to advice that the content transfer could not be done if no item has been selected.
IMHO it should be shown in any case.

> Line 4625:
Could be. Need to try.

> Line 4782:
IMHO “Page %1 of %2” in a docviewer is pretty unequivocal. Anyway, adding an hint for translators it's surely not a problem!

> There is a problem with file importing.
> You do a check to check if already exists a file with the same name. If there is, then > you import the file adding a numb at the end of the name.
> if(QFile:
> But I think you should add another check: if the new file it's the same you already > > imported, then don't import it.
> I think the best way to achieve this check is to use md5 function.
Again, same issue is in the gallery-app.

>src/app/qml/pdfView/PdfViewGotoDialog.qml
>+ Button {
>+ objectName:
>+ text: i18n.tr("GO!")
>+ color: UbuntuColors.orange
>Orange isn't anymore suggested by design guidelines[0]. I suggest to use green or gray
No problem, I'll change the color!

>src/app/qml/textView/TextView.qml
>Why do you use a TextArea with readOnly: true and not a Label?
Copy and paste! Even if the TextArea is read-only, they are still useful.

> src/app/quick/documentMode.cpp
> Could you please explain what are you trying to achieve with DocumentModel:
> Seems improvable, but I'm not sure I understand how you want to use it
When there's a change in the directory we're watching (e.g. document added or removed), we want to update the model according to the changes:
The code in that slot just removes all the entries from that directory and re-parse its content.
The two ints, n and m, are used to keep a note of the number of removed items, so that we can exactly remove the rows from the model (by doing so we avoid to have duplicated undefined rows).

About the ENUM, I will do in a next MP. At the moment it works, but I have no resources (most of all, time) for staying in-sync with the tasks I still need to do for the docviewer.
The same observation is also valid for the issues you reported that are related to the gallery-app.

For all the rest I will provide a fix hopefully today (max. tomorrow).
Thank you Riccardo for the huge review.