Merge lp:~verzegnassi-stefano/ubuntu-docviewer-app/document-hub2 into lp:ubuntu-docviewer-app/trunk
| Status: | Merged | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Approved by: | Stefano Verzegnassi on 2015-03-03 | ||||||||||||||||||||
| Approved revision: | 93 | ||||||||||||||||||||
| Merged at revision: | 86 | ||||||||||||||||||||
| Proposed branch: | lp:~verzegnassi-stefano/ubuntu-docviewer-app/document-hub2 | ||||||||||||||||||||
| Merge into: | lp:ubuntu-docviewer-app/trunk | ||||||||||||||||||||
| Diff against target: |
7414 lines (+4916/-1923) 75 files modified
CMakeLists.txt (+2/-2) com.ubuntu.docviewer.desktop.in.in (+3/-1) com.ubuntu.docviewer.url-dispatcher (+5/-0) debian/changelog (+12/-2) debian/control (+1/-1) docviewer-content.json (+4/-3) docviewer.apparmor (+7/-0) manifest.json.in (+3/-2) po/com.ubuntu.docviewer.pot (+155/-85) src/app/CMakeLists.txt (+22/-1) src/app/command-line-parser.cpp (+105/-0) src/app/command-line-parser.h (+56/-0) src/app/content-communicator.cpp (+188/-0) src/app/content-communicator.h (+69/-0) src/app/docviewer-application.cpp (+298/-0) src/app/docviewer-application.h (+92/-0) src/app/graphics/select-none.svg (+153/-0) src/app/graphics/select.svg (+158/-0) src/app/main.cpp (+10/-98) src/app/qml/ContentHubPicker.qml (+0/-71) src/app/qml/ContentHubProxy.qml (+0/-38) src/app/qml/DetailsPage.qml (+0/-58) src/app/qml/EmptyState.qml (+0/-59) src/app/qml/ErrorDialog.qml (+0/-32) src/app/qml/ImageView.qml (+0/-48) src/app/qml/ImageViewDefaultHeader.qml (+0/-74) src/app/qml/PageWithBottomEdge.qml (+0/-407) src/app/qml/PdfContentsPage.qml (+0/-60) src/app/qml/PdfView.qml (+0/-107) src/app/qml/PdfViewDefaultHeader.qml (+0/-96) src/app/qml/PdfViewDelegate.qml (+0/-95) src/app/qml/PdfViewGotoDialog.qml (+0/-60) src/app/qml/TextView.qml (+0/-70) src/app/qml/TextViewDefaultHeader.qml (+0/-82) src/app/qml/UnknownTypeDialog.qml (+0/-43) src/app/qml/WelcomePage.qml (+0/-42) src/app/qml/ZoomableImage.qml (+0/-155) src/app/qml/common/DetailsPage.qml (+58/-0) src/app/qml/common/ErrorDialog.qml (+32/-0) src/app/qml/common/UnknownTypeDialog.qml (+43/-0) src/app/qml/common/loadComponent.js (+38/-0) src/app/qml/common/utils.js (+34/-0) src/app/qml/documentPage/DeleteFileDialog.qml (+58/-0) src/app/qml/documentPage/DocumentEmptyState.qml (+34/-0) src/app/qml/documentPage/DocumentGridDelegate.qml (+178/-0) src/app/qml/documentPage/DocumentGridView.qml (+76/-0) src/app/qml/documentPage/DocumentListDelegate.qml (+108/-0) src/app/qml/documentPage/DocumentListView.qml (+156/-0) src/app/qml/documentPage/DocumentPage.qml (+76/-0) src/app/qml/documentPage/DocumentPageDefaultHeader.qml (+34/-0) src/app/qml/documentPage/DocumentPagePickModeHeader.qml (+63/-0) src/app/qml/documentPage/DocumentPageSelectionModeHeader.qml (+94/-0) src/app/qml/loadComponent.js (+0/-45) src/app/qml/pdfView/PdfContentsPage.qml (+60/-0) src/app/qml/pdfView/PdfView.qml (+109/-0) src/app/qml/pdfView/PdfViewDefaultHeader.qml (+96/-0) src/app/qml/pdfView/PdfViewDelegate.qml (+95/-0) src/app/qml/pdfView/PdfViewGotoDialog.qml (+60/-0) src/app/qml/textView/TextView.qml (+70/-0) src/app/qml/textView/TextViewDefaultHeader.qml (+82/-0) src/app/qml/ubuntu-docviewer-app.qml (+63/-37) src/app/qml/upstreamComponents/EmptyState.qml (+62/-0) src/app/qml/upstreamComponents/HeaderButton.qml (+65/-0) src/app/qml/upstreamComponents/ListItemWithActions.qml (+453/-0) src/app/qml/upstreamComponents/ListItemWithActionsCheckBox.qml (+25/-0) src/app/qml/upstreamComponents/MultipleSelectionGridView.qml (+199/-0) src/app/qml/upstreamComponents/MultipleSelectionListView.qml (+199/-0) src/app/qml/upstreamComponents/MultipleSelectionVisualModel.qml (+31/-0) src/app/qml/upstreamComponents/PageWithBottomEdge.qml (+407/-0) src/app/qml/utils.js (+0/-34) src/app/quick/documentmodel.cpp (+212/-0) src/app/quick/documentmodel.h (+89/-0) src/app/urlhandler.cpp (+70/-0) src/app/urlhandler.h (+43/-0) tests/autopilot/ubuntu_docviewer_app/tests/test_docviewer.py (+1/-15) |
||||||||||||||||||||
| To merge this branch: | bzr merge lp:~verzegnassi-stefano/ubuntu-docviewer-app/document-hub2 | ||||||||||||||||||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Alan Pope 🍺🐧🐱 🦄 | 2015-02-26 | Approve on 2015-03-03 | |
| Ubuntu Phone Apps Jenkins Bot | continuous-integration | Approve on 2015-03-03 | |
| Riccardo Padovani (community) | code | Approve on 2015-03-03 | |
|
Review via email:
|
|||
Commit Message
List of changes:
- Refactored project structure (too much QML files in the same folder)
- Added full content-hub support
- Uri handler support
- In-app browser
- Document Viewer has now rw permissions in HOME/Documents
- Dropped support for Image and Other file type
- Use new splash screen features
Description of the Change
I'm sorry for the length of the diff: bzr move didn't recognise the folder changes.
In order to help with the review, here's some information:
* All the C++ classes in src/app and src/app/quick needs a review.
* Files in src/app/
* All the files in src/app/
* Other QML files had just minor changes.
CHANGES:
- Refactored project structure (too much QML files in the same folder)
- Added full content-hub support
- Uri handler support
- In-app browser
- Document Viewer has now rw permissions in [HOME]/Documents
- Dropped support for Image and Other file type
- Use new splash screen features
MANUAL TESTING:
New features are still not covered by Autopilot tests. Before approving the MP, we need to manually execute some tests:
- Check if all the documents in HOME/Documents are shown in DocumentPage (NOTE: Only PDFs and file with mimetype=text/* are supported)
- Import content to the Document Viewer (browser-app and filemanager-app surely work, Dekko should also work)
- Export content to 3rd party applications (e.g. PdfjsViewer)
- Remove a document from user's Documents folder (tap-and-hold the list item, so the view switches to the selection mode).
Two conditions should be satisfied: the document should be deleted from user's Documents folder and the view should not display "undefined" entries).
- Add some document in the user's Documents folder while the application is running.
Two conditions should be satisfied: new documents should be listed in the view, and no "undefined" entries are displayed.
- Importing/exporting tests should be done with both the application running and closed.
- Check if the DocumentPage correctly switches between the three states of the page: default, selectionMode, pickerMode.
- Empty state: When Documents folder is empty, an empty state should be shown on the screen.
Ok, got the issue.
In docviewer-
Since in main.cpp[2] we set the ApplicationName (com.ubuntu.
"/usr/share/
instead of
"/usr/share/
I will fix it lately this night.
[1]: http://
[2]: http://
- 89. By Stefano Verzegnassi on 2015-02-27
-
Terribly bad-designed workaround for looking QML files path up in QStandardPaths:
:DataLocations
PASSED: Continuous integration, rev:89
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Pope 🍺🐧🐱 🦄 (popey) wrote : | # |
Not sure what I did wrong, but I built this with qtc and installed on my device and get this:-
File: qml/ubuntu-
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/
I'll solve it ASAP!
- 90. By Stefano Verzegnassi on 2015-02-27
-
Fixed revision 89
Ok, fixed my stupid mistake :-)
Tested on both desktop (14.10) and hammerhead (rtm)
PASSED: Continuous integration, rev:90
http://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Pope 🍺🐧🐱 🦄 (popey) wrote : | # |
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/
* 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://
* 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/
> "+" 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://
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. :)
| Riccardo Padovani (rpadovani) wrote : | # |
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
| Riccardo Padovani (rpadovani) wrote : | # |
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.
| Riccardo Padovani (rpadovani) wrote : | # |
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/
+ 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
###
src/app/
Why do you use a TextArea with readOnly: true and not a Label?
###
src/app/
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
DocumentModel:
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
| Riccardo Padovani (rpadovani) wrote : | # |
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/
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 filenameWithout
It comes from gallery-app
> Line 740: filenameWithout
Same as above.
> Line 757:
My fault here.
> Line 806: \brief ContentCommunic
Yes, it is. Forgot to change it (I did it the first time, but I lost the code) :P
> Line 849: return m_transfer-
TBH dunno, it's the same in the upstream code (gallery-app). I need to check.
> Line 983: bool ok = m_cmdLineParser
Nice question! Why I did it? Need to fix! :-)
> Line 1069: m_view-
Sure! (Even if the string is replaced by QML Page title)
> Line 1706: QcoreApplicatio
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.
You're right again!
>Line 3783: not sure which line do you mean?
>> If (import "../upstreamCom
The EmptyState item lives in ../upstreamComp
>> 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/
>+ 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 ...
| Riccardo Padovani (rpadovani) wrote : | # |
> 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 "../upstreamCom
> The EmptyState item lives in ../upstreamComp
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/
> >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/
> > 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!
- 91. By Stefano Verzegnassi on 2015-03-03
-
Remove 'debug' policy from apparmor profile
- 92. By Stefano Verzegnassi on 2015-03-03
-
Fixes for the minor issues found in the last review
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://
Executed test runs:
UNSTABLE: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Riccardo Padovani (rpadovani) wrote : | # |
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:
QCoreApplic
to:
QCoreApplic
makes the app look for the main QML file in the wrong path, since DataLocation is built as "/usr/share/
In this case, it is both[1].
[1] "/usr/local/
- 93. By Stefano Verzegnassi on 2015-03-03
-
Revert 'setOrganizatio
nName' change
I reverted to 'QCoreApplicati
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://
Executed test runs:
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Pope 🍺🐧🐱 🦄 (popey) wrote : | # |
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.
| Alan Pope 🍺🐧🐱 🦄 (popey) wrote : | # |
I didn't realise this was an intentional design decision based on what the gallery uses.


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