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

Disable TextFilter when no project is loaded #1171

Merged
merged 2 commits into from
Jan 19, 2025
Merged

Disable TextFilter when no project is loaded #1171

merged 2 commits into from
Jan 19, 2025

Conversation

rowo360
Copy link
Contributor

@rowo360 rowo360 commented Jan 12, 2025

I have noticed one issue with the new TextFilter (#1161) that I would like to fix with this PR:

We actually want the filters to be disabled when no project is loaded. And that's how we implemented it already for the OutcomeFilter.
However I missed to add this for the new TextFilter. This PR now makes up for this!
(File: DisplayStrategy.cs and NUnitTreeDisplayStrategy.cs)

@rowo360 rowo360 self-assigned this Jan 12, 2025
@rowo360
Copy link
Contributor Author

rowo360 commented Jan 12, 2025

While testing this functionality, I noticed another issue with the filters which doesn't work as expected:
We have added one button to show/hide the filter in the toolbar. This button should only be visible for the NUnit display strategy, but not for the Fixture/TestList display strategy.
That works when loading a test project and the strategy from the VisualState file is applied. But it won't work if the strategy is changed interactively - that's strange but I must have missed that one.

I fixed this issue by calling method UpdateViewCommands whenever the DisplayFormat changes. (File: TestCentricPresenter.cs)

@rowo360
Copy link
Contributor Author

rowo360 commented Jan 12, 2025

I added some tests for these use cases.
While looking at the existing tests in WhenSettingsChanged.cs I noticed that I miss to write proper Assert statements!
So I adapted also these existing tests.

@rowo360 rowo360 requested a review from CharliePoole January 18, 2025 09:31
CharliePoole
CharliePoole approved these changes Jan 18, 2025
View reviewed changes
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. If it's easy to do, I suggest a single view property to enable filters.

src/TestCentric/testcentric.gui/Presenters/DisplayStrategy.cs Outdated
@@ -76,6 +76,7 @@ public void OnTestUnloaded()
{
ClearTree();
_view.OutcomeFilter.Enabled = false;
_view.TextFilter.Enabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be cleaner for the view to have a single property to enable filters?

src/TestCentric/testcentric.gui/Presenters/NUnitTreeDisplayStrategy.cs Outdated
@@ -50,6 +50,7 @@ public override void OnTestLoaded(TestNode testNode, VisualState visualState)
SetDefaultInitialExpansion();

_view.OutcomeFilter.Enabled = true;
_view.TextFilter.Enabled = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@rowo360
Copy link
Contributor Author

rowo360 commented Jan 19, 2025

You are definitely right. This is much cleaner!
I'll add a new method void EnableFilterElements(bool enable) to the view.
Thank you for reviewing so carefully and suggesting improvements!

@rowo360 rowo360 merged commit 6d0ca17 into main Jan 19, 2025
2 checks passed
@rowo360 rowo360 deleted the issue-1161-2 branch January 19, 2025 16:43
@CharliePoole
Copy link
Contributor

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CharliePoole CharliePoole CharliePoole approved these changes

Assignees

@rowo360 rowo360

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rowo360 @CharliePoole