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

Add UI elements for textual filtering of tests #1166

Merged
merged 3 commits into from
Jan 11, 2025
Merged

Add UI elements for textual filtering of tests #1166

merged 3 commits into from
Jan 11, 2025

Conversation

rowo360
Copy link
Contributor

@rowo360 rowo360 commented Dec 19, 2024

This PR contributes to issue #1161 by adding a Textbox in the Filter ToolStrip to enable the textual filtering of tests.
Here's a screenshot:

Here's a list of the functionality:

  • If there's no input in the TextBox, it displays a default text (aka PlaceHolder text) to give a hint to the user about the purpose.
  • This default text is displayed in lightgray color (instead of black)
  • The textual filtering is not started immediately after each keystroke but with a short delay as soon as there's no further input
  • Identical to the outcome filter toolstrip: the Text filter toolstrip is only shown if the 'Show Filter' is activated. And it's only available in the NUnit display format.

Not included:

  • In some applications a 'X' is shown at the right side of a textual input field, which quickly deletes all text input. For example google search. I like this feature, however it's not supported by Windows forms out-of-the-box. A quick research provides some examples how to achieve this functionality, but I postponed it.
  • I noticed that the keystroke 'Ctrl+A' to select all text within the textbox, is not supported. A quick research shows up that a WindowsForm application setting can fix it, but I propose to handle this in a separate issue + PR.

Technical view:
The main contribution of this commit are the two new UI classes StretchToolStripTextBox and TextBoxElement.
The class StretchToolStripTextBox is only required to stretch the TextBox in the ToolStrip over the complete width. Without this implementation the TextBox width will remain fixed (for example 200 pixel). I'm glad that I found this solution in the microsoft documentation.
The class TextBoxElement handles the TextBox input. It implements the ISelection interface so that the presenter class don't depent on any UI elements, but just use the ISelection.SelectedItem and ISelection.SelectionChanged event.

@rowo360 rowo360 self-assigned this Dec 19, 2024
Copy link
Contributor

@CharliePoole CharliePoole left a comment

Choose a reason for hiding this comment

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

Looks good. I'm suggesting one change in the textbox element for consistency within the set of ui elements.

/// - show a PlaceHoder text if there's no text input
/// - Invoke the SelectionChanged event as soon as no further input is made within a short period of time.
/// </summary>
public class TextBoxElement : ISelection
Copy link
Contributor

Choose a reason for hiding this comment

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

ISelection seems like an odd interface to use here, since there are no choices from which to select. Do we need an IChanged interface? Also, why not inherit from ToolStripElement, the base class for such elements?

@rowo360
Copy link
Contributor Author

rowo360 commented Jan 10, 2025

I think I opted the ISelection interface simply because it exists and it almost fits. But at second glance I also see that I only use the ISelectionChanged event from the ISelection interface. The remaining properties of that interface: SelectedItem, SelectedIndex and the Refresh method are implemented with a dummy implementation or are throwing a NotImplemented exception.
So yes, that can be improved by introducing a new interface IChanged:

public interface IChanged : IViewElement
{
   event CommandHandler Changed;
}

I'll prepare a commit for this one.

I also thought again about inheriting the class from ToolStripElement: that'll work of course now. However I plan to reuse this TextBoxElement class in the 'Category filter dialog' next. In that use case the TextBoxElement class will be used with a simple TextBox instead of a ToolStripTextBox. That means that the inheritance from ToolStripElement won't fit for that.
One option might be to extract the common parts from base class ToolStripElement into a new base class so that it fits for usage of a TextBox and a ToolStripTextBox, but I wouldn't take this step now. Is this OK?

@rowo360
Copy link
Contributor Author

rowo360 commented Jan 10, 2025

This commit adds a new interface IChanged and use this interface instead of ISelection in class TextBoxElement.
I have given some thought to the name: is IChanged the best choice or would IViewElementChanged fit better?
And currently the event is simply named Changed. So it might not be clear which property of a IViewElement has changed (Enabled, Visible or Text). So an option might be to rename the event to TextChanged?
Just some thoughts... :-)

@CharliePoole
Copy link
Contributor

CharliePoole commented Jan 11, 2025

Regarding this idea...

I also thought again about inheriting the class from ToolStripElement: that'll work of course now. However I plan to reuse this TextBoxElement class in the 'Category filter dialog' next. In that use case the TextBoxElement class will be used with a simple TextBox instead of a ToolStripTextBox. That means that the inheritance from ToolStripElement won't fit for that.
One option might be to extract the common parts from base class ToolStripElement into a new base class so that it fits for usage of a TextBox and a ToolStripTextBox, but I wouldn't take this step now. Is this OK?

I first created the UI Elements to encapsulate various kinds of controls under the NUnit V2 GUI. For TestCentric, I wanted to use more modern UI components that had been introduced into Windows.

The Windows Forms Control and ToolStripItem classes each derive from Component and form two separate inheritance hierarchies. So for my UI Elements, I also created two hierarchies based on ControlElement and ToolStripElement respectively. In order to make them at least somewhat interchangeable, I created common interfaces to be implemented within each hierarchy.

One side issue... in Windows Forms, there is a third hierarchy based on MenuItem and I originally had a third group of elements to match. But with TestCentric, I moved completely to use of ToolStripMenuItem, which is wrapped by ToolStripMenuElement.

I think it's best to keep this structure of two hierarchies using some common functional interfaces, which are not connected with UI appearance at all. For example, ICommand represents issuing a command to do something and can be implemented by a menu item or a button. The visual representation can be changed without changing the presenter, which only knows there is an interface. So we can use inheritance to share common implementation, but only within each hierarchy.

UPDATE

I was initially confused because you had referred to your filterTextBox as a Control. I see now that it's a ToolStripItem and is on the other side of the hierarchy split from what I first thought. Windows ToolStripTextBox is, of course a specialized ToolStripControlHost, containing a textbox.

I think we should be careful about keeping the split between elements that represent ToolStripItems and those that represent Controls, but it's possible to do what you suggest by moving some of the specialized functiondown into the control itself. Let's not do it till it's needed however. :-)

Copy link
Contributor

@CharliePoole CharliePoole left a comment

Choose a reason for hiding this comment

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

This looks good to me. Suggestions are optional, just let me know what you do.

/// - show a PlaceHoder text if there's no text input
/// - Invoke the Changed event as soon as no further input is made within a short period of time.
/// </summary>
public class TextBoxElement : IChanged
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public class TextBoxElement : IChanged
public class ToolStripTextBoxElement : ToolStripElement, IChanged

ToolStripElement already implements many of the basic properties. Suggested name change is consistent with our naming of other tool strip elements.

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, you've convinced me 👍 !
I will rename this class to ToolStripTextBoxElement and derive it from base class ToolStripElement.

TextBox.Focus();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the timeout strictly necessary? I'm fairly accustomed to controls that do nothing until I tab away or otherwise change focus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This 'timeout' feature is definitely just a gimmick and not absolutely necessary.
When I was working with the text filters, I found it helpful that the results were displayed quickly without additional key stroke. And that I can continue typing immediately if I wasn't satisfied with the result. That's why I had the idea of introducing this timer. Of course, this is not a brand new idea, but some other programs use this in their search as well. So from my point of view it's beneficial for the user and I prefer to keept this feature. But of course we can remove it, if users are annoying by this feature...

@rowo360
Copy link
Contributor Author

rowo360 commented Jan 11, 2025

I first created the UI Elements to encapsulate various kinds of controls under the NUnit V2 GUI. For TestCentric, I wanted to use more modern UI components that had been introduced into Windows.

The Windows Forms Control and ToolStripItem classes each derive from Component and form two separate inheritance hierarchies. So for my UI Elements, I also created two hierarchies based on ControlElement and ToolStripElement respectively. In order to make them at least somewhat interchangeable, I created common interfaces to be implemented within each hierarchy.

Thanks for the explanation of the two derivation hierarchies: ControlElement and ToolStripElement. I have to admit that I haven't really noticed the difference yet. That's why I think your idea regarding the “Restructure Elements Directory” is a good one. The interfaces like ICommand or ISelection are of course the essential part and keep our presenter classes independent from the UI framework - I think this is a great approach and is implemented well.

@rowo360 rowo360 merged commit 8d60a2c into main Jan 11, 2025
2 checks passed
@rowo360 rowo360 deleted the issue-1161 branch January 11, 2025 16:26
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.

2 participants