Skip to content

Fixes #2120. TreeView: Hide cursor in single select mode by default, add cursor toggle to TreeViewFileSystem Scenario#2123

Merged
tig merged 2 commits intogui-cs:developfrom
tznind:cursor-option
Oct 22, 2022
Merged

Fixes #2120. TreeView: Hide cursor in single select mode by default, add cursor toggle to TreeViewFileSystem Scenario#2123
tig merged 2 commits intogui-cs:developfrom
tznind:cursor-option

Conversation

@tznind
Copy link
Copy Markdown
Collaborator

@tznind tznind commented Oct 21, 2022

Fixes #2120- Add cursor toggle to TreeViewFileSystem Scenario

@tig regarding issue #2120, there is already a feature for this (DesiredCursorVisibility). The default state of this is Default (i.e. it has a cursor). This PR adds a menu item to the scenario that lets user turn cursor on and off.

If you would like the default to be Invisible let me know and I can update this PR.

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@tig
Copy link
Copy Markdown
Member

tig commented Oct 21, 2022

What is the customer scenario where a cursor is useful on Treeview?

@tznind
Copy link
Copy Markdown
Collaborator Author

tznind commented Oct 21, 2022

What is the customer scenario where a cursor is useful on Treeview?

Primary use case is during multi selection to determine where the active row is. Even when multiple rows are selected there is still a 'primary' row which is the one tracked by SelectedObject and the one which the selection will be adjusted from if user uses Shift up/down.

You can see a similar effect in Visual Studio Code, but in this case the active row has a bright border.

activerowtree

I'm happy to go either way on this though. Setting the default to Invisible could be a good change for new users. The setting was originally added in response to #1559

@tig
Copy link
Copy Markdown
Member

tig commented Oct 21, 2022

What is the customer scenario where a cursor is useful on Treeview?

Primary use case is during multi selection to determine where the active row is. Even when multiple rows are selected there is still a 'primary' row which is the one tracked by SelectedObject and the one which the selection will be adjusted from if user uses Shift up/down.

You can see a similar effect in Visual Studio Code, but in this case the active row has a bright border.

activerowtree

I'm happy to go either way on this though. Setting the default to Invisible could be a good change for new users. The setting was originally added in response to #1559

Is there another scenario beyond multi-select where it's useful? Perhaps the right thing to do is only enable it when multi-select is active?

@tznind
Copy link
Copy Markdown
Collaborator Author

tznind commented Oct 21, 2022

Is there another scenario beyond multi-select where it's useful?

I can't really think another use case other than stylistic choice. Also because there is a public DesiredCursorVisibility property then removing it would be a breaking change. Changing its default value might also be considered breaking but is much more minor.

Perhaps the right thing to do is only enable it when multi-select is active?

As I was describing the use case I did think of this but I dislike 'magic' properties (where changing one public property MultiSelect auto changes another DesiredCursorVisibility). Especially you don't want :

tv.DesiredCursorVisibility = CursorVisibility.Invisible;
tv.MultiSelect = true;

To behave differently from:

tv.MultiSelect = true;
tv.DesiredCursorVisibility = CursorVisibility.Invisible;

Also relevant to this discussion is that MultiSelect defaults to true.

Let me know what you want to do, options seem to be:

  • Leave unchanged
  • Change DesiredCursorVisibility default to Invisible
  • Remove DesiredCursorVisibility and render cursor based solely on MultiSelect
  • Something else

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 21, 2022

@tznind see the TextView or the TextField where are conditions to show or hide the cursor. There is also a backup field where the cursor is restored (visible) accordingly with the user settings. What @tig suggest is only show the cursor if multi select is enabled and hide otherwise.

@tznind
Copy link
Copy Markdown
Collaborator Author

tznind commented Oct 21, 2022

There is also a backup field where the cursor is restored (visible) accordingly with the user settings. What @tig suggest is only show the cursor if multi select is enabled and hide otherwise.

Ok I think I understand... do you mean that DesiredCursorVisibility becomes a setting that only applies when multi select is on? And if single select is on then the DesiredCursorVisibility value is ignored and the cursor is suppressed?

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 21, 2022

Ok I think I understand... do you mean that DesiredCursorVisibility becomes a setting that only applies when multi select is on? And if single select is on then the DesiredCursorVisibility value is ignored and the cursor is suppressed?

Right, but take an account the the user may be configured the DesiredCursorVisibility to other than the default. Then you must use a private backup key to save the user setting before the change and use that backup key to restore it. For e.g. the user may be set a block visible cursor. Before you set to invisible you have to save the user setting in that field. Before restore to visible you have to set to the block cursor and not the default.

@tznind
Copy link
Copy Markdown
Collaborator Author

tznind commented Oct 21, 2022

Wouldn't it work just to change the get property:

/// <summary>
/// Get / Set the wished cursor when the tree is focused.
/// Only applies when <see cref="MultiSelect"/> is true.
/// </summary>
public CursorVisibility DesiredCursorVisibility {
	get { 
		return MultiSelect ? desiredCursorVisibility : CursorVisibility.Invisible;
	}
[...]

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 21, 2022

Probably yes.

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 22, 2022

@tznind don't forget to call Application.Driver.SetCursorVisibility (returnedValue) or Driver.SetCursorVisibility (returnedValue) and SetNeedsDisplay () after you get the value to effectively apply the changes if the view is focused.

@tznind
Copy link
Copy Markdown
Collaborator Author

tznind commented Oct 22, 2022

Ok I made the following changes of behavior:

TreeViewFileSystem now has menu items for toggling:

  • Cursor (on/off)
  • MultiSelect (on/off)

The changes to TreeView are:

  • Cursor now defaults to off (Invisible)
  • Cursor stores but ignores any changes to DesiredCursorVisibility while MultiSelect is off and always uses Invisible in this state.

Cursor visibility is set in the Enter event of the control and the set on DesiredCursorVisibility.

@tig tig changed the title Fixes #2120 Add cursor toggle to TreeViewFileSystem Scenario Fixes #2120. TreeView: Hide cursor in single select mode by default, add cursor toggle to TreeViewFileSystem Scenario Oct 22, 2022
@tig tig merged commit 015f0e8 into gui-cs:develop Oct 22, 2022
This was referenced Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TreeView: Cursor caret should not be visible

3 participants