[Folder View] Remember selected item when navigating in subfolders
ClosedPublic

Authored by thsurrel on Sep 29 2018, 9:18 PM.

Details

Summary

When using the folder view in a panel as a popup, being in list view
mode, getting into subfolders and back with arrow keys had a weird
behaviour: the currentItem would just stay the same accross folders.

This patch restores both the proper selected item and the scrolling
position when navigating back.

BUG: 359310

Test Plan
  • Add a folder view in a panel, configure it in list view mode
  • Enter a subfolder and go back

The folder you had gone into should be selected and the scrolling
position should be the same than when you entered it.

Diff Detail

Repository
R119 Plasma Desktop
Branch
arc_folderview3 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3672
Build 3690: arc lint + arc unit
thsurrel created this revision.Sep 29 2018, 9:18 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 29 2018, 9:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
thsurrel requested review of this revision.Sep 29 2018, 9:18 PM
ngraham added a subscriber: ngraham.EditedSep 29 2018, 10:27 PM

Does this fix a bugzilla ticket?

Having a second look, this bug is related:
https://bugs.kde.org/show_bug.cgi?id=359310
As it is, my patch does not fix that though, so far it remembers the selected item but not the scroll position.
I will try to improve my patch to fix both issues.

thsurrel planned changes to this revision.Sep 30 2018, 7:09 PM
thsurrel updated this revision to Diff 42624.Sep 30 2018, 8:32 PM

[Folder View] Restore scrolling position when going back

BUG: 359310

Gotta add BUG: 359310 to the Summary section, not a comment.

thsurrel edited the summary of this revision. (Show Details)Oct 1 2018, 7:26 AM
thsurrel edited the test plan for this revision. (Show Details)
thsurrel edited the test plan for this revision. (Show Details)Oct 1 2018, 11:08 AM
anthonyfieroni added inline comments.
containments/desktop/package/contents/ui/FolderView.qml
1113

Do we have grodView.model after back clicked? If we do in expanded view (that's when Folder View is embedded in the panel) goingBack will not be performed.

thsurrel added inline comments.Oct 1 2018, 1:04 PM
containments/desktop/package/contents/ui/FolderView.qml
1113

My understanding was that the gridView.model could be undefined only if the FolderModel was not ready. But if we navigated into a subfolder and we are asking to go back, than the FolderModel had to be up and running already. Could be safe to move this 'if' outside the 'else' though.

anthonyfieroni added inline comments.Oct 1 2018, 1:19 PM
containments/desktop/package/contents/ui/FolderView.qml
1113

Ok, then just embed Folder View in the panel (by right click on the panel, panel options -> add widgets or drag'n'drop it in) then navigate to check that should it goback to be applied as well.

thsurrel added inline comments.Oct 1 2018, 1:30 PM
containments/desktop/package/contents/ui/FolderView.qml
1113

Oh, it does, what you describe is the very use case this patch is about. I tested it :)
It's just that I was not entirely confident I got the case when gridView.model could be undefined right.

hein added a comment.Oct 6 2018, 1:14 AM

I like the goal here, but it's not a given the stored index is still valid when navigating back - the folder contents could have changed. It'd be hygienic to bound the access when popping from the history.

thsurrel updated this revision to Diff 42966.Oct 6 2018, 12:38 PM

Pop all history info at once when navigating back

In D15840#337446, @hein wrote:

I like the goal here, but it's not a given the stored index is still valid when navigating back - the folder contents could have changed. It'd be hygienic to bound the access when popping from the history.

Thanks for the review. I don't know if the modifications I just made correspond to what you had in mind.
But I tested both cases where A/ one of the parent folder we were into is deleted, in that case navigating back displays a message that this folder does not exist anymore, and B/ that the content of the folder changes. In case B, the Y position we are moving to might be incorrect if lots of files we added before. But I don't know what to do about that, and if it is worth the trouble ... :)

hein added a comment.EditedOct 8 2018, 5:02 PM
In D15840#337446, @hein wrote:

I like the goal here, but it's not a given the stored index is still valid when navigating back - the folder contents could have changed. It'd be hygienic to bound the access when popping from the history.

Thanks for the review. I don't know if the modifications I just made correspond to what you had in mind.
But I tested both cases where A/ one of the parent folder we were into is deleted, in that case navigating back displays a message that this folder does not exist anymore, and B/ that the content of the folder changes. In case B, the Y position we are moving to might be incorrect if lots of files we added before. But I don't know what to do about that, and if it is worth the trouble ... :)

What I mean is that setting a GridView to e.g. currentIndex=20 when there's only 10 items in the model is undefined behavior. I'd like you to bound it to GridView.count-1, and similar for contentY. :)

thsurrel updated this revision to Diff 43158.Oct 8 2018, 7:31 PM

Limit index and position to the current gridview state, in case some elements
have disappeared since we visited the parent folder.

hein added inline comments.Oct 8 2018, 11:48 PM
containments/desktop/package/contents/ui/FolderView.qml
1119

I don't think this works as intended. It means "only change to this contentY if it's lower than the current contentY".

I'm not really happy with the use of contentY to begin with, though, unfortunately. The problem is that contentY isn't really something that's a good idea to store permanently, because it's relative to originY, and originY can change arbitrarily depending on things like delegate inseration/removal outside the viewport, which can depend on the number of items in the model.

Could you try using the normalized values in gridView.visibleArea.* instead? If you use visibleArea.yPosition you can even forego the bounds check since it can't be out of bounds.

thsurrel updated this revision to Diff 43190.Oct 9 2018, 7:20 AM

Use gridView.visibleArea

hein added inline comments.Oct 10 2018, 6:23 AM
containments/desktop/package/contents/ui/FolderView.qml
194

You're still storing contentY here.

thsurrel updated this revision to Diff 43266.Oct 10 2018, 7:16 AM

I'm not sure how I managed to lose this bit when preparing this.
Thanks for your patience, hein, I have been doing a pretty poor job on this patch.

hein accepted this revision.Oct 10 2018, 7:43 AM

No worries, thanks for the patch :)

This revision is now accepted and ready to land.Oct 10 2018, 7:43 AM
This revision was automatically updated to reflect the committed changes.