Fix details view not closed when viewing a file in its folder

[danxuliu]

As far as I understand, #1448 occurs because setActiveView is called with silent: true. This prevents the itemChanged event to be triggered, which would have otherwise closed the details view.

I suppose (but I do not really know) that setting the active item is done in silent mode to avoid an unnecessary reload of the / directory in _onNavigationChanged (as the plugin then loads the folder of the selected file). Therefore, I fixed it by closing the details view in the plugin itself, just like it would have been closed by _onNavigationChanged.

This does not tackle the other problem mentioned in the issue, which is having several details view with the same app-sidebar id. It could be solved by giving each details view its own id, which would require a change in the CSS as currently #app-sidebar is used to position its contents, or by using a single details view for all the file lists, although it will probably involve deeper changes as right now every file list expect its own details view to be available for its lifetime.

Anyway, despite being "wrong" the current approach of several details view with the same id works, and with a possible redesign of the dashboard being discussed a proper fix could be just wasted effort...

(Some files of this pull request conflict with those in other pull request that I have opened; I will rebase this pull request or the other one as needed if they got merged)

[mention-bot]

@danxuliu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke, @MorrisJobke and @rullzer to be potential reviewers.

[blizzz]

This does not tackle the other problem mentioned in the issue, which is having several details view with the same app-sidebar id.

It should be re-used, no additionally created. There should not be more than a sidebar.

Your solution works however, but I am not sure about the UX. Keeping the sidebar open seems reasonable and less noisy. cc @nextcloud/designers

[jancborchardt]

with a possible redesign of the dashboard being discussed a proper fix could be just wasted effort

@danxuliu just FYI: That redesign is simply a proposal, so any bugfix is definitely not wasted effort. :)

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@51da044). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4461   +/-   ##
=========================================
  Coverage          ?   54.19%           
  Complexity        ?    21640           
=========================================
  Files             ?     1327           
  Lines             ?    82826           
  Branches          ?     1312           
=========================================
  Hits              ?    44886           
  Misses            ?    37940           
  Partials          ?        0

[danxuliu]

@blizzz

This does not tackle the other problem mentioned in the issue, which is having several details view with the same app-sidebar id.

It should be re-used, no additionally created. There should not be more than a sidebar.

Your solution works however, but I am not sure about the UX. Keeping the sidebar open seems reasonable and less noisy. cc @nextcloud/designers

Personally I would also prefer to keep the details view open when viewing the file in its folder. However, I made it to close because I thought it was more consistent with current behaviour (even if I do not like that behaviour ;) ). Right now, changing the file list or even changing the directory in the same file list closes the details view (although in this case it could be argued that the context of the file is kept even when changing to a different file list while in other cases it is not, so in this case the details view should be kept open even if in other cases it is closed).

But there was a technical reason to close the details view, and it was that as far as I know there is no way to copy the state from the sidebar of the original file list to the sidebar of the final file list, and I did not want to implement that to just remove it later when the sidebar was fixed to have a single one ;)

@jancborchardt

That redesign is simply a proposal, so any bugfix is definitely not wasted effort. :)

Then I can try to fix the multiple sidebar issue if you want. Although I think that it would be better to merge (if it is approved, of course ;) ) the fix for the user-facing bug here (to get it fixed as soon as possible) and then work in the "conceptual" bug in a different pull request.

[blizzz]

An acceptence test is failing, otherwise i am ok with it.

[MorrisJobke]

An acceptence test is failing, otherwise i am ok with it.

@danxuliu Could you look into this?

[danxuliu]

It is a false positive (I have ensured running the tests in my computer).

[blizzz]

It is a false positive (I have ensured running the tests in my computer).

Cool, thx. I am good. Then we need only a second reviewre.

[MorrisJobke]

Tested and works 👍