-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix rendering of the sidebar in Files app #12577
Merged
MorrisJobke
merged 5 commits into
master
from
fix-rendering-of-the-sidebar-in-files-app
Nov 22, 2018
Merged
Fix rendering of the sidebar in Files app #12577
MorrisJobke
merged 5 commits into
master
from
fix-rendering-of-the-sidebar-in-files-app
Nov 22, 2018
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When a view is rendered it should not be concerned with where it is going to be placed in the document; in general this should be a responsibility of the object using the view. Moreover, when the details view is rendered it should simply prepare a skeleton that includes the root elements provided by the plugins; those elements will be updated by the plugins as needed when a file or a tab is selected. Finally, the details view should not be explicitly rendered. The rendering removes the previous elements, but that is needed only when the details view is in a dirty state, that is, when new plugins were added since the last time that it was rendered. However, that dirty state is internally handled, and the view is automatically rendered again if needed when a file info is set. Due to all that the details view is no longer explicitly rendered when updating it with a different file. Also, as each file list has its own details view, and each details view has its own element, but there can be only one details view/sidebar element in the document, when the file list updates the details view it also replaces the current one in the document with its own details view if needed (that is, if it is not the current one already). Besides that, when the element of a details view is replaced with the element of a different details view the old one should be detached from the document, but never removed. Otherwise the event handlers would not work when that element is attached again later (when changing to a different section in the Files app and then going back to the previous one). Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu
requested review from
rullzer,
MorrisJobke,
ChristophWurst,
blizzz,
juliusknorr and
skjnldsv
November 22, 2018 06:53
skjnldsv
approved these changes
Nov 22, 2018
The only CI failure is due to git clone failed. |
MorrisJobke
approved these changes
Nov 22, 2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works 👍
/backport to stable14 |
The backport to stable14 failed. Please do this backport manually. |
This was referenced Nov 22, 2018
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #12320
This pull request fixes the regressions regarding the sidebar introduced in #12231 and #10509 (which, in turn, fixed the regression of the sidebar introduced in #9982).
When a view is rendered it should not be concerned with where it is going to be placed in the document; in general this should be a responsibility of the object using the view.
Moreover, when the details view is rendered it should simply prepare a skeleton that includes the root elements provided by the plugins; those elements will be updated by the plugins as needed when a file or a tab is selected.
Finally, the details view should not be explicitly rendered. The rendering removes the previous elements, but that is needed only when the details view is in a dirty state, that is, when new plugins were added since the last time that it was rendered. However, that dirty state is internally handled, and the view is automatically rendered again if needed when a file info is set.
Due to all that the details view is no longer explicitly rendered when updating it with a different file. Also, as each file list has its own details view, and each details view has its own element, but there can be only one details view/sidebar element in the document, when the file list updates the details view it also replaces the current one in the document with its own details view if needed (that is, if it is not the current one already).
Besides that, when the element of a details view is replaced with the element of a different details view the old one should be detached from the document, but never removed. Otherwise the event handlers would not work when that element is attached again later (when changing to a different section in the Files app and then going back to the previous one).
Essentially the problems with buttons not working come from this last point (which was also increased by rendering the details view after setting the file info). Excuse me for not giving more details, but it is quite late ;-) (yes, late, not early; for me it is still yesterday :-P ).
Nevertheless, for reference here is the explanation of what was the problem with comments: it did not come from rendering the view twice, but from setting the file info for one file and then setting the file info for a different file almost immediately; rendering the details view after setting the file info fixed the problem only because the same file info was set twice ;-)
CommentsTabView.setFileInfo
resets the collection and then fetches the next page, which triggers a sync request. However,reset
does not abort current pending requests (see pull 1325 in https://github.com/jashkenas/backbone (I am not linking it directly to prevent the reference from polluting that thread)). Thus, ifsetFileInfo
is called with two different models once the second one is setup the response for the first one will be received, and the comments in that response will be included in the second model.Due to all that it may be possible that further bugs with the comments are lurking in the shadows, but in most cases it should work fine ;-)
I have added acceptance tests for the issue that was fixed by #12231 (comments), issues caused by #12231 (favoriting from the details view), and issues caused (or, at least, not yet fixed) by #10509 (tags and opening and closing the details view). Note, however, that the tests for tags still fail sometimes due to a race condition; this is another issue to be fixed in a different pull request.