Huge update #7

nfsprodriver · Sep 3, 2017

Now using cibersheep's svg icon
Switched to click version from launchpad
Fixed Network navigation
Included Dan Chapman's new-listitem branch
Replaced context menu by swipe actions bars
Fixed clipboard clearing
Fixed hidden 'go to' bar
Make hidden files option savable (with workaround starting app in /home dir)
Work with multiple files
New selection bar with icons
Added new build instructions (finished?)

Flohack74 · Sep 3, 2017

Wow 180 files changed, I can´t review this manually. @mariogrip can we add Jenkins here so that we have ci test result? Then I would say lets install the click and test it a bit...

nfsprodriver · Sep 3, 2017

I 'edited' not really 180 files. Most is because I've rewritten them from the click repo. You may just review the qml and the root scripts.

sverzegnassi · Sep 3, 2017

In absolute terms, it is not much huge either. I'll try to have a look during the following week. The changes to the build system are not new to me, since File Manager uses some tool and configuration that I originally wrote for DocViewer

sverzegnassi · Sep 4, 2017

sverzegnassi reviewed Sep 4, 2017

View changes

I had a first look at the PR, comments are tagged with [FIXME], [TODO], or [INFO].

[INFO]'s are just notes or questions (see e.g. README-NewBuildInstructions.md).

What I didn't check:

.po files: we can regenerate them anytime, if necessary
Debian packaging stuff. I see you just reverted them to a previous revision, and we don't need them atm
Snap packaging stuff. Same as debian pkg.

What I still have to check:

Changes to QML code. Filemanager-app code is chaotic in many parts. I need to take some time in order to properly review your changes.
Testing the app on desktop and phone.

Anyway, I can already say that it looks okay overall, and only minor corrections are required. I think that once we're ready for merging this PR, we could publish an update on the OpenStore.

Then, we could evaluate if it still make sense to fork and rewrite the app from scratch - as I was doing - or it's better to push my changes to this branch, after we improved the code quality a bit.

There are many things in this app (e.g. tab management, hidden pages with dead links, usage of deprecated QML modules, bad theming support, etc.) that would urgently need some fix. I hope to find some time during the UbuCon, since it might be an opportunity for discussing this with more contributors.

Thanks for your PR.

[TODO] Add .gitignore with the same rules

[FIXME] We should get revision info from git, as the code is hosted on GitHub now. Please refer to: sverzegnassi/terminal-app@8016f12

[INFO] Just adding a reminder, I see that rev. 590 of File Manager trunk on Launchpad added this.
While it has been added as part of the snap-only support, it might still make sense to leave detailed licensing info. Not a blocker though.

[INFO] Looks okay, however Ubuntu SDK chroot kits have been deprecated in favor of LXD containers. In any case, we can eventually fix this after the merge.

Further question: this looks to be an alternative way for building the app without using Ubuntu SDK/IDE. It's legit, but I'm wondering why not using clickable, for instance.

We should have a deeper look it building the app after merging this, I think.

[FIXME] Might make sense to keep this line, as the "Apps" scope can also search between app keywords

[INFO] Just a cumulative comment for all the debian/* files. I haven't checked them, at the first sight, it's just the result of a 'bzr revert -r590'.
I don't think we'll build .deb packages of filemanager-app in a short term, so that's okay.

[INFO] I know it was already there, and that JSON does not support comments by design. However, we might safely remove it.

[FIXME] See comment for CMakeLists.txt.

[FIXME] Remove x-source and x-test, since they are no longer necessary

[INFO] I totally skip the review of the 'snap' bits, as they are not part of our requirements (for now at least). Also, snap is still evolving, and we might have to edit them again in the future.
Looks okay. :)

[FIXME] Add it back. I guess Canonical engineers have tracked the origin this C++ header, I'd prefer to preserve all the licensing info where appropriate.

[FIXME] Same here. Keep licensing details

Flohack74 · Sep 4, 2017

Just a note: Weblate does regenerate the po´s out of pot automatically now, so in general please do not touch po files. It will result in a merge conflict on the weblate server which needs manual intervention ;)

nfsprodriver · Sep 4, 2017

@sverzegnassi I'll check your hints. @Flohack74 should I revert to repo's current pos?

Flohack74 · Sep 4, 2017

If you have change in po, then yes please :) - But there is another thing, File Manager translations are blocked on Weblate so far. Shall I open them again @sverzegnassi ? I think we could work on the current version a bit.

sverzegnassi · Sep 4, 2017

@Flohack74 No problem, feel free to unlock them. I agree with you: at this point, it might be worth to do some change to the current codebase, I'm going to give it a look while I'm at UbuCon@Paris

nfsprodriver · Sep 6, 2017

The PR is finished from my side. Ready to be reviewed and merged :)

NeoTheThird · Sep 11, 2017

From what I can see it looks good, but i'd like a final review from @sverzegnassi... ;)

nfsprodriver · Sep 11, 2017

At any questions, please ask me :)

sverzegnassi · Sep 12, 2017

I started to review the code while I was in Paris, but sadly my chroot environment was broken, therefore I was not able to test it on the phone.
I'll have a check tomorrow, as I come back home.

sverzegnassi · Sep 14, 2017

sverzegnassi requested changes Sep 14, 2017 • edited

View changes

Ok, found some time for testing the app on the phone.

For what I saw, I would request some further change:

I see that "/home/phablet" has been hardcoded here and there. The PlacesModel object called 'userplaces' - which is declared in the main QML file - is globally available from all the other QML subfiles. Please use it. By the way, I don't understand why some action should only be visible when we're inside '/home/phablet'
"/home" has been hardcoded as default folder for filemanager-app. The folder is not available when full access has not been granted to the app. Conventionally, default folder shold be "/home/< username >", so please use 'userplaces.locationHome'
COPYING.LGPL is missing
ContentHub support is broken. It is required by some app currently available on OpenStore.
The logic of those ListItemActions seems a bit opaque (specifically, the 'visible' property)
Define 'text' property for ListItemActions' actions, so that users can see a description of those actions when they open the contextual menu by clicking w/ right button of the mouse.

Not blocking (i.e. we can do the following tasks when this PR is merged)

Not really convinced by the behavior of the header when there is a selection available. Default actions are not visible as long as the clipboard is not cleared.
When I long-press a file in the list, I would expect that file to be automatically selected.
Info in FileDetailsPopover.qml are not fully visible, since 'wrapMode' does not work
We should implement multiple selection also for the grid view
If the deletion of multiple files gets aborted, filemanager-app should preserve the current selection (i.e. not exiting selection mode)
I would expect some visual hint when I select some file for a cut action (e.g. opacity = 0.5)
Use a bool property outside delegates for evaluating if path is writable inside new ListItem actions

nfsprodriver · Sep 14, 2017

About the hardcode init path: This is just a workaround to make the newly saveable hidden files option be seen as soon as possible you start to use the app. This can be reverted, but this should be fixed. Didn't know about the broken ContentHub, for me the parts with this I tested worked. I'll check this out later. Thanks!

sverzegnassi · Sep 15, 2017

@nfsprodriver

Are we talking about filemanager-app being launched in "/home/"?
If so, I can see the issue: if show_hidden_files is true at start-up, hidden files are not anyway visible.
I will have a look and try to fix this; if possible, I would like to remove the usage of U1DB since it makes the app a bit more complex than necessary.
What about the visibility of ListItemActions in FolderListView.qml?
I see you check whether it's a hidden file in "/home/phablet" or not. The binding looks a bit heavy and redundant, and can potentially reduce the performance of the application.
If I have a look at FolderListPage.qml, e.g. line 98, for the same action in the grid view, I see a more linear binding:
Action { visible: folderListPage.__pathIsWritable }
Is there any specific reason why this wasn't work for you?

nfsprodriver · Sep 16, 2017

@sverzegnassi to point 1: Exactly, I had many attempts to get around that issue. I will have some time next week to check this also. To point 2: This is just a security propose so no user deletes important config files. We should discuss this. Subfolders are deletable.

sverzegnassi · Sep 16, 2017

Ok, I'm having a look right now, seems there's something wrong in the C++ code of the FolderListModel. I will push a PR to your branch soon.
I would restore it to the earlier policies (i.e. just check if path is writable). It's an issue that was already there, and we can't prevent users from gaining full access by typing their password and deleting system files anyway. We could - in the future - just check if full access has been granted, and toggle hidden files visibility according to that.

I'd really like to get this PR merged soon, so we can start working on the UI, which is full of magic numbers and colors. The app was not in a good state - IMHO - so we probably need to make further pull requests before releasing a new version on the OpenStore.

nfsprodriver · Sep 16, 2017

I'll remove the ~/.* policies. We should discuss that later. Can you tell me more about the ContentHub issue?

sverzegnassi · Sep 17, 2017

@nfsprodriver I've found a few issues (tested with PdfViewer):

PdfViewer (in the OpenStore) requests imports of single files. However, filemanager-app seems not to respect this settings, and allow multiple files selections.
If the content sharing request has been aborted from PdfViewer, filemanager-app should react and quit the content-hub selection UI.
If file-manager-app was already launched, and the new selection controls have been previously used, I was unable to see the content-hub controls for picking a file. The app just kept me showing the new selection controls, instead of content-hub controls. However, I'm not anymore able to reproduce it, since now the application just crashes when I select filemanager as a peer.

sverzegnassi · Sep 17, 2017

Ok, let's keep ContentHub out of the PR for now. ContentHub logic is mixed up with other things at the moment, so I prefer to clean up the code a bit first. Let's merge it!
Thanks again, @nfsprodriver !

sverzegnassi · Sep 17, 2017

sverzegnassi approved these changes Sep 17, 2017

View changes

nfsprodriver · Sep 17, 2017

About the ContentHub: Maybe there are just some little things to fix. Maybe I did some dumb mistakes which changed the basic behaviour :)

Flohack74 · Sep 17, 2017

Wow it was merged! :)

NeoTheThird · Sep 17, 2017

Wow, thanks so much guys, this is awesome! Do you want me to publish a new release to the OpenStore?

sverzegnassi · Sep 18, 2017

@NeoTheThird No, not yet. Personally, I'm not anymore sure about releasing an "intermediate" version before we finish rewriting the app.

There are a lot of small things that doesn't work as expected, duplicated code that works differently each from the other, and a huge amount of implicit name resolving which can be potentially dangerous.
I'd like to address these things before releasing anything, then updating the UI to something that looks a bit better would be just a minor work. :)

Reasonably, ETA is end of September. \o/

NeoTheThird · Sep 18, 2017

Ok cool, looking forward to it! :)

nfsprodriver added some commits Sep 3, 2017

Huge update

a2dccdd

executable scripts

82b7bb9

hotfix

c32984d

Update README-NewBuildInstructions.md

ba60140

Update README-NewBuildInstructions.md

89edaa3

Oops, reverting version bump...

5129b0c

Work with multiple files etc.

1933c4c

This was referenced Sep 4, 2017

Open

Filemanger crash browsing network #1

Closed

cut and select all #3

Action visibility fixes

ba7aea9

nfsprodriver added some commits Sep 4, 2017

Attempt implementing stefano's hints

85c97ff

Attempt reverting language files

de5f353

Removed BZR lines

9187a49

Added git as chroot dependency to get GIT_REVNO

1d46938

Added hidden file policy also for multiple files

f3311e8

Hotfix

088dc9c

NeoTheThird requested a review from sverzegnassi Sep 11, 2017

sverzegnassi added some commits Sep 14, 2017

Restore COPYING.BSD

5e50ede

Update .gitignore

7647fa7

Bump app version

c22a8b7

Fix building with Ubuntu IDE
As we did in ubports/terminal-app

732ce60

Added 'text' properties to ListItemActions
TODO for the future: FolderListPage.qml already provides the same actions inside 'actionSelectionPopover'. We should define those actions once, then use them where required.

14bb6fc

Reverted default folder to /home/stefano

57289fb

Revert ~/.* policy
We will discuss that...

ce319b2

sverzegnassi referenced this pull request Sep 17, 2017
Merged
Update from Weblate. #9

sverzegnassi merged commit 82492f4 into ubports:master Sep 17, 2017

nfsprodriver referenced this pull request in nfsprodriver/filemanager-app Sep 25, 2017

Merge pull request #7 from nfsprodriver/patch-1
Added (un)select all feature

a15dcdd

-        -set(DESKTOP_DIR ${CMAKE_INSTALL_DATADIR}/applications)
-        -install(FILES ${CONTENT_HUB_JSON} DESTINATION ${CMAKE_INSTALL_FULL_DATADIR}/content-hub/peers/ RENAME ubuntu-filemanager-app)
+        +if(CLICK_MODE)
+        +    if(NOT DEFINED BZR_SOURCE)

@@ -5,10 +5,7 @@ Terminal=false
          Exec=@EXEC@
          Icon=@ICON@
          _Name=File Manager
-        -_Keywords=folder;manager;explore;disk;filesystem;

+        +{
+        +    "description": "File Manager application",
+        +    "framework": "ubuntu-sdk-15.04.4",
+        +    "_comment": "Do not change this. Architecture is automatically replaced by cmake",

+        +    "maintainer": "Ubuntu App Cats <ubuntu-touch-coreapps@lists.launchpad.net>",
+        +    "name": "com.ubuntu.filemanager",
+        +    "title": "File Manager",
+        +    "version": "0.4.@BZR_REVNO@",

+        +    "name": "com.ubuntu.filemanager",
+        +    "title": "File Manager",
+        +    "version": "0.4.@BZR_REVNO@",
+        +    "x-source": {

		@@ -1,10 +0,0 @@
		-*.qmlproject.user

		@@ -1,26 +0,0 @@
		-Copyright (c) 2012, Robin Burchell

		@@ -0,0 +1,49 @@
		+Building a click package

		@@ -1,15 +0,0 @@
		-# vim:syntax=apparmor

ubports/filemanager-app

Join GitHub today

Huge update #7

Conversation

nfsprodriver commented Sep 3, 2017 • edited Edited 1 time nfsprodriver Sep 3, 2017

nfsprodriver added some commits Sep 3, 2017

Flohack74 commented Sep 3, 2017

nfsprodriver commented Sep 3, 2017

sverzegnassi commented Sep 3, 2017 • edited Edited 1 time sverzegnassi Sep 3, 2017

This was referenced Sep 4, 2017

sverzegnassi reviewed Sep 4, 2017 View changes

Flohack74 commented Sep 4, 2017

nfsprodriver commented Sep 4, 2017

Flohack74 commented Sep 4, 2017

sverzegnassi commented Sep 4, 2017

nfsprodriver added some commits Sep 4, 2017

nfsprodriver commented Sep 6, 2017

NeoTheThird requested a review from sverzegnassi Sep 11, 2017

NeoTheThird commented Sep 11, 2017

nfsprodriver commented Sep 11, 2017

sverzegnassi commented Sep 12, 2017

sverzegnassi added some commits Sep 14, 2017

sverzegnassi requested changes Sep 14, 2017 • edited View changes

nfsprodriver commented Sep 14, 2017

sverzegnassi added some commits Sep 15, 2017

sverzegnassi commented Sep 15, 2017

nfsprodriver commented Sep 16, 2017

sverzegnassi commented Sep 16, 2017

sverzegnassi and others added some commits Sep 16, 2017

nfsprodriver commented Sep 16, 2017

sverzegnassi commented Sep 17, 2017

sverzegnassi referenced this pull request Sep 17, 2017

sverzegnassi commented Sep 17, 2017

sverzegnassi approved these changes Sep 17, 2017 View changes

sverzegnassi merged commit 82492f4 into ubports:master Sep 17, 2017

nfsprodriver commented Sep 17, 2017

Flohack74 commented Sep 17, 2017

NeoTheThird commented Sep 17, 2017

sverzegnassi commented Sep 18, 2017

NeoTheThird commented Sep 18, 2017

nfsprodriver referenced this pull request in nfsprodriver/filemanager-app Sep 25, 2017

nfsprodriver commented Sep 3, 2017 •

Edited 1 time

nfsprodriver
Sep 3, 2017

sverzegnassi commented Sep 3, 2017 •

Edited 1 time

sverzegnassi
Sep 3, 2017

sverzegnassi reviewed Sep 4, 2017

View changes

sverzegnassi requested changes Sep 14, 2017 • edited

View changes

sverzegnassi approved these changes Sep 17, 2017

View changes

sverzegnassi merged commit `82492f4` into ubports:master Sep 17, 2017