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

always open existing setting item in 'settings-view:open' command #1043

Merged
merged 5 commits into from Feb 26, 2018

Conversation

Projects
None yet
3 participants
@t9md
Contributor

t9md commented Jan 24, 2018

Fix #723

Description of the Change

Condition #723 happens is

  • When workspace have two or more panes and setting-view is already opened in currently not active pane.
  • User invoke settings-view:open(it try to open atom://config)

Why happens is

  • atom.worksace.open try to open already opened setting-view item also in different pane

Changes

  • If setting-view is already opened in workspace, activate it's pane in opener so that workspace.open always open already opened item in workpace.
  • This hacky help is necessary since singleton settingView item is opened as different URI(workpace.open cannot open existing seting-view item by URI).
  • It eventually simulate searchAllPanes: true option in workspace.open.

Plus(I can remove this refactoring part if requested)

  • Add SettingsView::showPanelForURI method which open matching URI's panel

Background investigation why I chose this approach is explained in #723 (comment).

Alternate Designs

Currently settingsView is singleton.
Quit singleton and instantiate settingsView always is another solution.
As a result user can open multiple settingsView on every pane like one file can open in multiple editors.
But I think this is too much/big change.

Benefits

User no loner see exception

Possible Drawbacks

Nothing

Applicable Issues

#723

t9md added some commits Jan 24, 2018

🎨 refactoring, add showPanelForURI to open panel from URI
Remove duplicate logic + reducing size of main.js is good to reduce
startup impact(since setting-view have no activationCommand in pkg.json)
@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Jan 26, 2018

This might also fix atom/atom#12498 because the repro steps include getting into a bad state by opening the settings-view in two different panes.

@ungb

This comment has been minimized.

Contributor

ungb commented Feb 26, 2018

Thanks @t9md for this contribution!

@ungb ungb merged commit 693f406 into atom:master Feb 26, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment