D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:6736bdaa4bbd: [Folder View] Remember selected item when 
navigating in subfolders (authored by thsurrel).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15840?vs=43266=43282

REVISION DETAIL
  https://phabricator.kde.org/D15840

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderView.qml

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Eike Hein]

hein accepted this revision.
hein added a comment.
This revision is now accepted and ready to land.


  No worries, thanks for the patch :)

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arc_folderview3 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel updated this revision to Diff 43266.
thsurrel added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15840?vs=43190=43266

BRANCH
  arc_folderview3 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15840

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderView.qml

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> FolderView.qml:194
>  function doCd(row) {
> -history.push(url);
> +history.push({"url": url, "index": gridView.currentIndex, 
> "contentY": gridView.contentY});
>  updateHistory();

You're still storing contentY here.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel updated this revision to Diff 43190.
thsurrel added a comment.


  Use gridView.visibleArea

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15840?vs=43158=43190

BRANCH
  arc_folderview3 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15840

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderView.qml

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> FolderView.qml:1119
> +setSelected(positioner.map(gridView.currentIndex));
> +gridView.contentY = Math.min(lastPosition.contentY, 
> gridView.contentY);
>  }

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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel updated this revision to Diff 43158.
thsurrel added a comment.


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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15840?vs=42966=43158

BRANCH
  arc_folderview3 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15840

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderView.qml

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Eike Hein]

hein added a comment.


  In D15840#337571 , @thsurrel wrote:
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel updated this revision to Diff 42966.
thsurrel added a comment.


  Pop all history info at once when navigating back

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15840?vs=42624=42966

BRANCH
  arc_folderview3 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15840

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderView.qml

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Eike Hein]

hein added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in FolderView.qml:1114
> 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.

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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> thsurrel wrote in FolderView.qml:1114
> 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.

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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in FolderView.qml:1114
> 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.

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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik, hein
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> FolderView.qml:1114
>  onListingCompleted: {
>  if (!gridView.model && plasmoid.expanded) {
>  gridView.model = positioner;

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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma, broulik
Cc: anthonyfieroni, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel updated this revision to Diff 42624.
thsurrel added a comment.


  [Folder View] Restore scrolling position when going back
  
  BUG: 359310

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15840?vs=42568=42624

BRANCH
  arc_folderview3 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15840

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderView.qml
  containments/desktop/package/contents/ui/FolderViewLayer.qml

To: thsurrel, #plasma
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Nathaniel Graham]

ngraham added a comment.


  Does this bug a bugzilla ticket?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D15840

To: thsurrel, #plasma
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15840: [Folder View] Remember selected item when navigating in subfolders

[Thomas Surrel]

thsurrel created this revision.
thsurrel added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
thsurrel requested review of this revision.

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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arc_folderview3 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15840

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderView.qml
  containments/desktop/package/contents/ui/FolderViewLayer.qml

To: thsurrel, #plasma
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart