Merge lp://qastaging/~dpm/ubuntu-filemanager-app/merge-settings into lp://qastaging/ubuntu-filemanager-app

Proposed by David Planella
Status: Needs review
Proposed branch: lp://qastaging/~dpm/ubuntu-filemanager-app/merge-settings
Merge into: lp://qastaging/ubuntu-filemanager-app
Diff against target: 604 lines (+137/-367)
5 files modified
src/app/qml/filemanager.qml (+8/-29)
src/app/qml/ui/FolderListPage.qml (+6/-15)
src/app/qml/ui/PlacesPopover.qml (+0/-149)
src/app/qml/ui/SettingsPage.qml (+123/-19)
src/app/qml/ui/ViewPopover.qml (+0/-155)
To merge this branch: bzr merge lp://qastaging/~dpm/ubuntu-filemanager-app/merge-settings
Reviewer Review Type Date Requested Status
Jenkins Bot continuous-integration Needs Fixing
Arto Jalkanen Needs Fixing
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve
Review via email: mp+240543@code.qastaging.launchpad.net

Commit message

Merges the view options and the current settings view into a single settings page.

Description of the change

Merges the view options and the current settings view into a single settings page.

I've done some cleanup and removed some dead code. However, one related thing that still needs to be fixed is how settings are stored and the UI updated when they change. I see two issues with the current code:
- Settings are properties of either MainView or the FolderListPage component. This makes it a bit unwieldy to set from the Settings page. They should all probably be properties of one single component.
- The way to put settings into effect once they've been changed from the Settings page is to use the reloadSettings() function. Is there a more efficient way to trigger an update of the properties?

Any feedback will be really welcome!

Notes:
- Not an issue, but the use of a dark theme triggers bug 1389112 (UbuntuShape shown for ItemSelector)
- Not a big issue, but again the use of a dark theme triggers bug 1389115 (ItemSelector ticks not visible)
- Wrapping the settings into i18n.tr() is not a good idea, as if a translation is done after one particular release, then the database will be out of sync with the UI. Need to find a solution to store the settings in English while showing them translated in the UI.

To post a comment you must log in.
Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Arto Jalkanen (ajalkane) wrote :

I tried changing (in tablet mode, using List view) for example to "Sort by date". It had no effect. I tried changing "Sort order" - no change.

Another problem is that these settings are not accessible at all in "phone" mode. Probably an artifact of the former "feature" that "Settings" page was only available in "tablet" mode.

review: Needs Fixing
322. By David Planella

Made settings visible on phone mode

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Arto Jalkanen (ajalkane) wrote :

Now settings available in phone mode, but there's still the problem that changing sort mode doesn't work (in neither tablet nor phone mode).

Console gets this error when trying to change those Settings:

file:///home/arto/coding/review/build-merge-settings-Desktop-Default/src/app/qml/ui/SettingsPage.qml:95: ReferenceError: fileView is not defined

Revision history for this message
Arto Jalkanen (ajalkane) :
review: Needs Fixing
Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)

Unmerged revisions

322. By David Planella

Made settings visible on phone mode

321. By David Planella

Removed dead code, wrapped settings in a flickable, created layout for settings

320. By David Planella

Merged different settings into one page

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches