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

[workspace] do not re-open an already currently opened workspace #5632

Merged
merged 1 commit into from
Jul 4, 2019
Merged

[workspace] do not re-open an already currently opened workspace #5632

merged 1 commit into from
Jul 4, 2019

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Jul 3, 2019 •
edited
Loading

Fixes #5630

Description

  • fixes an issue that allows users to open a workspace that is currently opened. If the user had the preference "workspace.preserveWindow" set to false it would open an identical workspace in a new tab, and if it was set to true it would reload the workspace. Instead, an additional
    check is performed to determine if the current workspace and the new destination are equal, and if they are a NOOP is instead performed by returning undefined.
  • fixes issue present in OPEN and OPEN_WORKSPACE for both the browser and electron based applications.

The PR was tested with both folders, files, and multi-root workspaces on both the browser and electron.

How to test

  1. open a workspace (ex: A)
  2. attempt to re-open the workspace A using the menu File and Open
  3. the workspace should not be re-opened in a new tab or the current one should not be reloaded depending on the preference workspace.preserveWindow

Signed-off-by: Vincent Fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added bug bugs found in the application workspace issues related to the workspace labels Jul 3, 2019
@vince-fugnitto vince-fugnitto self-assigned this Jul 3, 2019
@kittaakos
Copy link
Contributor

It did not work for me. Am I missing something?
screencast 2019-07-03 15-49-07

@vince-fugnitto
Copy link
Member Author

It did not work for me. Am I missing something?

No you're not missing anything, I'm missing something as I only updated the OPEN and not also the OPEN_WORKSPACE which can be both used.

@kittaakos
Copy link
Contributor

I propose checking the URI only and not the entire FileStat. Why? Because we do not want to compare the lastModification. When we open a new workspace we kind of cache the FileStat (which could be good or bad I do not know) as is; with the lastModification. When I create a new file in the root, I will get back a new, updated FileStat but no one updates the cached FileStat representing the workspace root. When we compare them, they won't be equal as the lastModification will be different.

Steps to reproduce.

  • Open a WS.
  • Reopen the same WS => NOOP => ✅
  • Create a new file inside the WS root.
  • Reopen the same WS => Opens a new workspace => ❌

Thoughts?

@vince-fugnitto
Copy link
Member Author

I propose checking the URI only and not the entire FileStat...

Sure makes sense!

@vince-fugnitto
Do not re-open an already currently opened workspace
511fbab 
Fixes #5630

- fixes an issue that allows users to open a workspace
that is currently opened. If the user had the preference
`"workspace.preserveWindow"` set to `false` it would open
an identical workspace in a new tab, and if it was set to
`true` it would reload the workspace. Instead, an additional
check is performed to determine if the current workspace and
the new destination are equal, and if they are a NOOP is
instead performed by returning `undefined`.
- fixes issue present in `OPEN` and `OPEN_WORKSPACE` for both
the browser and electron based applications.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
kittaakos
kittaakos approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I have tested it again and it worked. Thank you!

@vince-fugnitto
Copy link
Member Author

@kittaakos thank you!

@vince-fugnitto vince-fugnitto merged commit 37c9659 into master Jul 4, 2019
@vince-fugnitto vince-fugnitto deleted the vf/GH-5630 branch July 4, 2019 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kittaakos kittaakos kittaakos approved these changes

@lmcbout lmcbout Awaiting requested review from lmcbout

Assignees

@vince-fugnitto vince-fugnitto

Labels
bug bugs found in the application workspace issues related to the workspace
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(Re-)Opening a currently opened workspace should be NOOP
2 participants
@vince-fugnitto @kittaakos