Skip to content
This repository was archived by the owner on Oct 4, 2021. It is now read-only.

[GetToCode] Adds noshellEnabled/noshellVisible in CommandCodon to disable items based in WelcomeScreen #8240

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

netonjm
Copy link
Contributor

@netonjm netonjm commented Jul 19, 2019

This PR adds a new feature in CommandCodon to enable/disable menu items

Now we can add in the addon manigest

  • noshellEnabled = "[true|false]"
  • noshellVisible = "[true|false]"

and we'll disable/hidden menus when there is no shell (this is the WelcomeScreen mode)

This properties could be added to:

  • Command (single or an array of items)
  • Category

And in case of group the value will affect it's children.

Fixes VSTS #935559 - [Shell] Some menus should be disabled in only GTC mode

Disabling layout Command group

Screenshot 2019-09-03 at 17 02 33

Disabling Debug category group

Screenshot 2019-09-03 at 17 01 23

@netonjm netonjm requested a review from sevoku July 19, 2019 17:28
@netonjm netonjm requested a review from slluis as a code owner July 19, 2019 17:28
@netonjm netonjm self-assigned this Jul 19, 2019
@Therzok
Copy link
Contributor

Therzok commented Jul 19, 2019

Why do we need this?

@Therzok
Copy link
Contributor

Therzok commented Jul 19, 2019

What commands should we actually have in GTC mode? Maybe we should actually conditionally add commands for when we only show GTC. As in, a whitelist, not a blacklist.

@Therzok
Copy link
Contributor

Therzok commented Jul 19, 2019

I feel we should just declare a different extension point into which we put commands which are visible without the Ide being there.

@netonjm netonjm changed the title [GetToCode] Adds a PostUpdate in CommandHandler to base logic based in inheritance [GetToCode][Prototype] Adds a PostUpdate in CommandHandler to base logic based in inheritance Jul 20, 2019
@sevoku
Copy link
Member

sevoku commented Jul 22, 2019

I agree with @Therzok, but I don't think that we need an additional extension point for this (would be cumbersome to maintain). A better approach would be a simple opt-in in CommandCodon.
Something like:

  • noshellEnabled = "[true|false]"
  • noshellVisible = "[true|false]"

or we use one prop like noshell = "disabled|disabled-visible"

could look like this to opt in to show in disabled state:

<Command     id = "MonoDevelop.Debugger.DebugCommands.DebugApplication"
 noshellEnabled = "false"
 noshellVisible = "true"
           [...]
/>

Also I wouldn't delegate the logic to the command handlers, but do it all in CommandManager. If a command has no opt-in, it wouldn't run the update handler at all but just set the appropriate values in the CommandInfo (so they get picked up correctly by the menu logic).

@Therzok Therzok added this to the 8.3 milestone Aug 21, 2019
@netonjm netonjm changed the title [GetToCode][Prototype] Adds a PostUpdate in CommandHandler to base logic based in inheritance [GetToCode] Adds noshellEnabled/noshellVisible in CommandCodon to disable items based in WelcomeScreen Sep 3, 2019
}

internal void InternalUpdate (CommandArrayInfo info)
{
Update (info);

if (Ide.IdeApp.Workbench.Visible)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this query gtk? We update command handlers in bulk, might be a bit too much to query this 40 times for a menu open on the UI thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we could cache this value in WelcomePageService

Base automatically changed from master to main March 9, 2021 14:17
@akoeplinger akoeplinger changed the base branch from main to master March 15, 2021 17:02
Base automatically changed from master to main March 15, 2021 17:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants