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.

Sign up for GitHub

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

Jump to bottom

[navigator] Added toolbar to disable auto sync #5986

Merged
merged 1 commit into from
Sep 4, 2019
Merged

[navigator] Added toolbar to disable auto sync #5986

merged 1 commit into from
Sep 4, 2019

Conversation

svenefftinge
Copy link
Contributor

@svenefftinge svenefftinge commented Aug 19, 2019 •
edited
Loading

What it does

This adds a toolbar command to the navigator, that allows to easily disable the auto-sync between editor and navigator.

How to test

Open some editors and switch between them.
See how the navigator reveals the corresponding items all the time.
Disable the new toolbar item in the navigator to disable the sync.
Switch between editors and see how the navigator stays as is.
Enable the new toolbar item and see how the navigator syncs again with the current editor.

Review checklist

Reminder for reviewers

@akosyakov
Copy link
Member

PR template is missing 🙄

@akosyakov
Copy link
Member

Should not it auto reveal when enabled? It keeps showing the old file regardless of a currently selected editor.

I usually use it feature in Eclipse as follow:

  • disable auto-reveal
  • open a needed file
  • select auto-reveal, file is revealed

It does not happen with this PR.

vince-fugnitto
vince-fugnitto reviewed Aug 20, 2019
View reviewed changes
packages/navigator/src/browser/navigator-preferences.ts
@@ -22,16 +22,16 @@ import { createPreferenceProxy, PreferenceProxy, PreferenceService, PreferenceCo
export const FileNavigatorConfigSchema: PreferenceSchema = {
'type': 'object',
properties: {
'navigator.autoReveal': {
'explorer.autoReveal': {
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a breaking change note in the changelog for the rename?

@akosyakov akosyakov added the navigator issues related to the navigator/explorer label Aug 20, 2019
akosyakov
akosyakov approved these changes Aug 20, 2019
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It works nicely now. Mentioning in CHANGELOG that a preference was renamed would be good.

@svenefftinge
Copy link
Contributor Author

svenefftinge commented Aug 20, 2019 •
edited
Loading

I'd like to rebase this on top of #5953 and add the toggle to the menu. In addition, if the user has disabled the auto-sync, I'd like to add an icon to the toolbar that would just reveal the current active editor's file in the navigator, once. Anton pointed out that is how it's done in IntelliJ and sounds better.

@vince-fugnitto any estimates on when you are finishing #5953?

@vince-fugnitto
Copy link
Member

@vince-fugnitto any estimates on when you are finishing #5953?

@svenefftinge

There are some parts that I am unsure about (blocking issues).
I can always commit a PR that includes a subset or limited commands which we can later add to in the future.

Some of the commands currently do not show up (in comparison to 0.8.0) because they expect selections or even a workspace node. I assume these commands will need to be adjusted following the newest implementation of the Explorer.

@akosyakov
Copy link
Member

akosyakov commented Aug 20, 2019 •
edited
Loading

@vince-fugnitto Let's split it then? first low hanging fruits, then the rest with another PR?

@vince-fugnitto
Copy link
Member

@vince-fugnitto Let's split it then? first low hanging fruits, then the rest with another PR?

I was just about to prepare the PR just for that :)

@svenefftinge svenefftinge force-pushed the se/navigator_no_sync branch 2 times, most recently from 9674a64 to 2fc0dfc Compare August 28, 2019 08:09
@svenefftinge
Copy link
Contributor Author

svenefftinge commented Aug 28, 2019 •
edited
Loading

I've mode the icon into the menu now. Please review

@akosyakov
Copy link
Member

@svenefftinge it does not compile

akosyakov
akosyakov reviewed Aug 30, 2019
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

@svenefftinge it does not work for me, I never see this action in the context menu:

Screen Shot 2019-08-30 at 15 56 55

@svenefftinge
Copy link
Contributor Author

ouch, sorry I didn't test again after fixing the compile issue.
Now it's there:
Screenshot 2019-08-31 at 08 26 17

akosyakov
akosyakov approved these changes Sep 1, 2019
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

It does work now and code looks good.

But I wonder how can one quickly reveal a file without enabling the preference? Only using the quick palette? Maybe we could have at least a keybinding for Reveal In Files command?

@svenefftinge
[navigator] Added command and toolbar menu item to disable auto sync
deacd0b 
Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>
@svenefftinge svenefftinge merged commit 811c298 into master Sep 4, 2019
@svenefftinge svenefftinge deleted the se/navigator_no_sync branch September 4, 2019 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

@akosyakov akosyakov akosyakov approved these changes

Assignees
No one assigned
Labels
navigator issues related to the navigator/explorer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@svenefftinge @akosyakov @vince-fugnitto