Merge lp://qastaging/~verzegnassi-stefano/ubuntu-docviewer-app/document-hub2 into lp://qastaging/ubuntu-docviewer-app/trunk

FAILED: Continuous integration, rev:88
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/173/
Executed test runs:
    UNSTABLE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2139
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2139/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-utopic-amd64-ci/70
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-vivid-amd64-ci/76

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/173/rebuild

Ok, got the issue.
In docviewer-application.cpp[1] we use the old code used to find out where the main qml file is located (from line 138).
Since in main.cpp[2] we set the ApplicationName (com.ubuntu.docviewer), the app looks for the qml file in the wrong folder:

"/usr/share/com.ubuntu.docviewer/qml/ubuntu-docviewer-app.qml"
instead of
"/usr/share/ubuntu-docviewer-app/qml/ubuntu-docviewer-app.qml"

I will fix it lately this night.

[1]: http://bazaar.launchpad.net/~verzegnassi-stefano/ubuntu-docviewer-app/document-hub2/view/head:/src/app/docviewer-application.cpp
[2]: http://bazaar.launchpad.net/~verzegnassi-stefano/ubuntu-docviewer-app/document-hub2/view/head:/src/app/main.cpp

PASSED: Continuous integration, rev:89
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/174/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2151
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2151/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-utopic-amd64-ci/71
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-vivid-amd64-ci/77

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/174/rebuild

Not sure what I did wrong, but I built this with qtc and installed on my device and get this:-

File: qml/ubuntu-docviewer-app.qml does not exist at any of the standard paths!

Tried on vivid flo and rtm (utopic) krillin.

Definitely my fault! The "terribly bad-designed workaround" is really terribly bad-designed...

The folders used in debian packaging are different than the ones of the click package.
Should change the debian rules, so app files are installed in /usr/share/com.ubuntu.docviewer

I'll solve it ASAP!

Ok, fixed my stupid mistake :-)
Tested on both desktop (14.10) and hammerhead (rtm)

PASSED: Continuous integration, rev:90
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/176/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2170
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2170/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-utopic-amd64-ci/73
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-vivid-amd64-ci/79

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/176/rebuild

A few comments.

* Really like that documents just appear when I copy them into ~/Documents. Also like that deleting them removes them from disk. This helps with managing disk space! Nice one!

* If we only look in ~/Documents then it doesn't find files on my SD card which are in /media/phablet/2541-1C26/Documents - and as we no longer have the "+" button in the toolbar, I can't add them to the list using file manager.

* Long press to delete brings buttons in the toolbar which are not right-aligned. There is a gap (I assume where the list/grid toggle button was) in the toolbar. I think the buttons should appear on the right. http://people.canonical.com/~alan/screenshots/device-2015-02-28-155804.png

* Long press to delete is not easily discovered and doesn't match the rest of the platform which uses swipe right to delete (see music / contacts / alarms etc). Can we have long press for multi-select delete _and_ swipe right for single document delete?

> A few comments.
>
> * Really like that documents just appear when I copy them into ~/Documents.
> Also like that deleting them removes them from disk. This helps with managing
> disk space! Nice one!

Glad you like it!

> * If we only look in ~/Documents then it doesn't find files on my SD card
> which are in /media/phablet/2541-1C26/Documents - and as we no longer have the
> "+" button in the toolbar, I can't add them to the list using file manager.

Well, you can open files from the file manager itself: Document Viewer will be shown in the list of the available destinations for the trasfer, and a local copy will be created in ~/Documents.
ATM I prefer not to support external storages, since this MP is already huge for testing, and I still need to find out the best solution for being notified of changes in the list of removable medias.

> * Long press to delete brings buttons in the toolbar which are not right-
> aligned. There is a gap (I assume where the list/grid toggle button was) in
> the toolbar. I think the buttons should appear on the right.
> http://people.canonical.com/~alan/screenshots/device-2015-02-28-155804.png

I saw it some time ago. It seemed to me that it was unpredictable, and doesn't happen any time the headerState switches.
Sure I'll try to fix it.

> * Long press to delete is not easily discovered and doesn't match the rest of
> the platform which uses swipe right to delete (see music / contacts / alarms
> etc). Can we have long press for multi-select delete _and_ swipe right for
> single document delete?

Design reasons. I've already added a comment in the code in order to remember that I have to activate the swipe-to-delete gesture.
Sure we can have it, but I need to find equivalent action for the delegate in the grid view mode.
I'm open to any suggestions. :)

Ok, it's good for me then.

I read the first 5000 lines of diff, and I left some comments.

Later I'll finish the diff, and then I'll test the app. Maybe I'll do another code review because it's huge

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::exists(destination))

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.

http://doc.qt.io/qt-5/qml-qtqml-qt.html#md5-method

Last 2403 lines of diff (rev 90).
Unfortunately they aren't here on Launchpad. So I locally merged this branch in the trunk, then I used qdiff, I report here interesting pieces:

src/app/qml/pdfView/PdfViewGotoDialog.qml
+ Button {
+ objectName:"GOButton"
+ text: i18n.tr("GO!")
+ color: UbuntuColors.orange

Orange isn't anymore suggested by design guidelines[0]. I suggest to use green or gray

###
src/app/qml/textView/TextView.qml

Why do you use a TextArea with readOnly: true and not a Label?

###
src/app/quick/documentmodel.cpp

Could you please explain what are you trying to achieve with DocumentModel::_q_directoryChanged?
Seems improvable, but I'm not sure I understand how you want to use it

DocumentModel::parseDirectoryContent
item.dateDiff = 0;
Probably is better to add a ENUM for this

Great work Stefano, congrats!

All issues I pointed out are minor, and the improvement is huge :-)

I'm going to test it

[0]https://design.ubuntu.com/apps/building-blocks/buttons

Mhh, I tried to launch it on vivid desktop 64bit.

It compiles like a charm, but then the app doesn't start, both via CLI and via QtCreator.
It is stucked on

./src/app/ubuntu-docviewer-app
APP_ID isn't set, the handler can not be registered

I have all packages reported by the README.

What am I doing wrong?

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

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

I completely understand :D

> >Line 3783: not sure which line do you mean?
> >> If (import "../upstreamComponents”)
> The EmptyState item lives in ../upstreamComponents

Oh, right, my fault here!

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

You have a point here, I agree with you

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

Aha, right! Didn't think about it, sorry

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

Oh, I see, makes sense.
I think could be improvable, but atm works well, so it's ok

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

I fully understand that, there are simply too much work to do :-)

> For all the rest I will provide a fix hopefully today (max. tomorrow).

Ok, I'll do another quick review after then I'll approve it, thanks!

> Thank you Riccardo for the huge review.

Thanks to you for the amazing work!

Just a note for Riccardo, when he will review the latest changes:
 * I've added some comment in the code for the things I didn't fix yet (e.g. issues related to the gallery-app).
 * Please double check translation changes (e.g. i18n.tr("dd MM yyyy, hh:mm"), since I'm not completely sure if I did it right)

FAILED: Continuous integration, rev:92
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/177/
Executed test runs:
    UNSTABLE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2211
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2211/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-utopic-amd64-ci/74
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-vivid-amd64-ci/80

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/177/rebuild

Now code looks good to me.

There are some FIXME, but they don't cause any error, they are only minor code refactoring you can do in a second moment.

Since this branch has a lot of improvements, I think is good enough to land, I'm sure you will do other branches for little fixes :-)

As we discussed yesterday, I'm not able to launch it on vivid desktop, but could be a my problem.
Anyway, I wasn't able to review the UI/UX. Code is good, but I leave to popey the top approving :-)

Changing:
    QCoreApplication::setOrganizationDomain("com.ubuntu.docviewer")
to:
    QCoreApplication::setOrganizationName("com.ubuntu.docviewer")

makes the app look for the main QML file in the wrong path, since DataLocation is built as "/usr/share/<APPNAME>", where <APPNAME> (according to Qt docs) "is usually the organization name, the application name, or both, or a unique name generated at packaging."

In this case, it is both[1].

[1] "/usr/local/share/com.ubuntu.docviewer/com.ubuntu.docviewer/qml/ubuntu-docviewer-app.qml"

I reverted to 'QCoreApplication::setOrganizationDomain".

Here's the reason of my choice: the code has been on testing for a few weeks, and even if the line was wrong, it used to work with no issue both on desktop and on devices.
Since I have no UT device at the moment - my brand new (a.k.a. "refurbished") hammerhead has been shipped today - I can't properly test if any further change will cause some regression.

Content-hub used to work, as all the rest of the application did.
Because of this, I assume that everything is ok for a release.

I should get my new Nexus 5 in max. 2-3 days: when it will happen, I'll be able to fix this part of the code.

PASSED: Continuous integration, rev:93
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/178/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2212
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2212/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-utopic-amd64-ci/75
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-docviewer-app-vivid-amd64-ci/81

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-docviewer-app-ci/178/rebuild

Seems launching a pdf from another application via content hub broke.

* Open browser
* Search for pdf + weather
* Click a random pdf file
* Content hub popup, select docviewer
* Wait for download
* Wait for popup, choose 'Open'
* Docviewer launches, but document list is shown, the document doesn't load.

I didn't realise this was an intentional design decision based on what the gallery uses.