Skip to content
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

Sync file selection state between tab group and outline view #1296

Merged
merged 14 commits into from
Jun 14, 2023

Conversation

dscyrescotti
Copy link
Contributor

Description

This PR includes a bug fix for aligning file selection state on the Project Navigator (outline view) with the current active tab in editor's tab group.

Related Issues

#1294

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

Screen.Recording.2023-06-04.at.9.20.25.AM.mov

@austincondiff
Copy link
Collaborator

This is great work! I just have several questions:

  • Does it scroll to the newly selected item when changed?
  • Does it expand the parent folders if not expanded to show the selected item?

One other thing worth mentioning is that In Xcode the project navigator selection is not synced with the active editor tab. That being said, I think this should be a setting. The question is, what do we want the default to be?

This setting is found in VS Code:
image

I think we keep it simple. We'd have a setting under General that says: "Automatically reveal open file in Project Navigator" and then to the right have a toggle switch.

@CodeEditApp/maintainers any thoughts?

@dscyrescotti
Copy link
Contributor Author

@austincondiff It won't scroll to the active file or reveal like Xcode. IMO, with regard to having a setting for auto reveal, it will be great so that the user can adjust based on what he prefers.

@dscyrescotti
Copy link
Contributor Author

@austincondiff Oh my bad, I found a setting for auto reveal preference and it is off by default. It will reveal the file if it is turned on.
Screenshot 2023-06-04 at 10 35 06 AM

@austincondiff
Copy link
Collaborator

austincondiff commented Jun 4, 2023

Oh you are right, I guess the work has already been done.

In my testing (on main):

  • the selected file is synced with the project navigator selection whether this setting is on or not
  • It does not scroll to the selected file
  • It does not expand parent folders to show the selected file

This behavior should be disabled if this setting is off, The selection should be scrolled to when changed and parent folders should be opened to reveal the selection.

@austincondiff
Copy link
Collaborator

I also get this...
image
I cannot reliably reproduce, but sometimes when switching I get an alert that says, Cannot find file. I click okay and another alert comes up. This repeats. I have to quit the app and rebuild to get it to stop.

@dscyrescotti
Copy link
Contributor Author

@austincondiff Let me check it on my side

@dscyrescotti
Copy link
Contributor Author

I got the similar alert message after renaming untitled file. I believe this issue is the one you got. This is due to object mismatch between datasource of the outlineview and one from workspace file manager, resulting row index -1.

Screen.Recording.2023-06-04.at.1.52.55.PM.mov

@dscyrescotti
Copy link
Contributor Author

I updated PR with the following changes:

  1. sync file selection between tab group and outline view - 77405e8
  2. restore file selection when folder is expanded - dd5526d
  3. ensure not to display "Cannot find file" alert repeatedly - 5a60230
  4. update selection after reloading datasource - fd9712a
Screen.Recording.2023-06-10.at.4.10.34.PM.mov

@austincondiff
Copy link
Collaborator

austincondiff commented Jun 10, 2023

@dscyrescotti Very nice! Does it scroll to the newly selected item if out of view?

@dscyrescotti
Copy link
Contributor Author

Yes it does if the reveal setting is on... I forgot to include in recording. Here it is.

Screen.Recording.2023-06-11.at.12.02.03.AM.mov

@austincondiff
Copy link
Collaborator

austincondiff commented Jun 11, 2023

That is great, but the selected file should probably go to the middle of the scrollview instead of the top or bottom. Try pressing ⌘⇧J in Xcode to see how this works in Xcode. This might be approaching scope-creep though. If you would like, we can merge this and create a separate issue to do this.

@dscyrescotti
Copy link
Contributor Author

Yes it would be better to tackle them separately as this PR focuses on file sync. So the things left to do are to move the selected file to the middle (only if the setting is on) like VSCode and add ⌘⇧J action to manual reveal like Xcode.

@austincondiff austincondiff requested a review from a team June 11, 2023 19:33
austincondiff
austincondiff previously approved these changes Jun 11, 2023
Copy link
Collaborator

@austincondiff austincondiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Great work!

@austincondiff
Copy link
Collaborator

One thing I noticed as well is it is almost impossible to collapse a folder that contains the active file. When changing tabs, it expands. It should remain collapsed until returning to that tab.

thecoolwinter
thecoolwinter previously approved these changes Jun 14, 2023
Copy link
Collaborator

@thecoolwinter thecoolwinter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good once the changes requested by other maintainers are done!

@austincondiff austincondiff enabled auto-merge (squash) June 14, 2023 19:56
@austincondiff austincondiff disabled auto-merge June 14, 2023 19:56
@austincondiff austincondiff merged commit 37b451b into CodeEditApp:main Jun 14, 2023
@austincondiff
Copy link
Collaborator

@allcontributors add @dscyrescotti for code

@allcontributors
Copy link
Contributor

@austincondiff

I've put up a pull request to add @dscyrescotti! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants