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

Adjust behavior of all close tab commands #7278

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

Hanksha
Copy link
Contributor

@Hanksha Hanksha commented Mar 5, 2020

Fixes #7268, adjust behavior of all close tab commands so that they respect the widget.title.closable property.

What it does

This PR improves the close tab commands so that they take into account the widget.title.closable property. It means that a widget cannot be closed using any of the commands if widget.title.closable === false.

The behavior is as follow:

  • Close and Close Tab in Main Area are enabled when the current widget is closable
  • Close Others and Close Other Tabs in Main Area are enabled if at least one other widget is closable
  • Close All and Close All Tabs in Main Area enabled if at least one widget is closable
  • Close to the Right is enabled if at least one widget to the right is closable

How to test

I added a sample view to the examples so you can open the Sample Unclosable View and toggle the checkbox in it to set title.closable and try the logic described above.

Review checklist

Reminder for reviewers

@akosyakov
Copy link
Member

As far as I understood this PR only tries to prevent closing from user perspective, from programming perspective one can still close. I think it is reasonable, just checking.

@akosyakov akosyakov added the shell issues related to the core shell label Mar 5, 2020
@Hanksha
Copy link
Contributor Author

Hanksha commented Mar 5, 2020

@akosyakov Yes it only affects the commands, one can still close the tab programmatically using the ApplicationShell methods.

@akosyakov
Copy link
Member

@Hanksha fyi https://help.github.com/en/enterprise/2.16/user/github/managing-your-work-on-github/closing-issues-using-keywords

@Hanksha Hanksha changed the title [#7268] Adjust behavior of all close tab commands Adjust behavior of all close tab commands Mar 5, 2020
Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Great! A few remarks.

Copy link
Contributor

@westbury westbury 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 this and it works fairly well. I did note the following:

  1. When a view is not closable (and there are other closable views), the action 'Close Others' is disabled. I would expect it to be enabled.
  2. It looks like 'close others' should only close views within the same location. We need to be sure this is all consistent.

bind(WidgetFactory).toDynamicValue(ctx => ({
id: SampleView.ID,
createWidget: () => ctx.container.get<SampleView>(SampleView)
}));
Copy link
Contributor

@westbury westbury Mar 5, 2020

Choose a reason for hiding this comment

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

This function is going to get rather big if lots of stuff is added to it. Also it is not clear which lines relate to which example. Same comment applies to api-samples-contribution. It seems to me we should have a single function call here for each example, so here we have just

bindDynamicLabelProvider(bind);
bindUnclosableView(bind);

Then have a folder for each example where these functions are implemented. I know this is not how it was set up before, but now you are adding the second example it would be helpful if you were able to improve the structure here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I see what you mean, I'll improve that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it a try, tell me what you think.

@Hanksha
Copy link
Contributor Author

Hanksha commented Mar 6, 2020

  1. When a view is not closable (and there are other closable views), the action 'Close Others' is disabled. I would expect it to be enabled.
  2. It looks like 'close others' should only close views within the same location. We need to be sure this is all consistent.
  1. I cannot reproduce it, "Close Others" is disabled only when there are no other closable views.
  2. Yes that's right, I changed it to behave that way.

@Hanksha Hanksha force-pushed the GH-7268 branch 4 times, most recently from 1f82b50 to aa2135a Compare March 6, 2020 02:07
…t they respect the widget.title.closable propertySigned-off-by: Vivien Jovet <[email protected]>
@akosyakov
Copy link
Member

@spoenemann please merge

@westbury
Copy link
Contributor

westbury commented Mar 6, 2020

  1. cannot reproduce it, "Close Others" is disabled only when there are no other closable views.

I was misunderstanding what was the same or a different area. It's working fine.

Copy link
Contributor

@westbury westbury left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks also for changing the view title from 'Sample'. I forgot to mention that but you did it anyway. I appreciate your keeping the examples package tidy too, as that was not really part of this PR.

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

Successfully merging this pull request may close these issues.

Can close widget tab via context menu even if set as not closeable
4 participants