Add UI elements for test outcome filtering

[rowo360]

This PR contributes to issue #1148 by providing UI elements to filter tests by their outcome.

Here's a screenshot for some visual insight:

Here's a list of the functionality:

• Overall there are 4 options for outcome filtering: All, Passed, Failed or 'Not run' tests
  The two additional outcomes 'Noninclusive' and 'Skipped/Warning' are not considered so far. We can discuss if we should add the support for them right now or wait until it's requested!
• By default the 'All option is checked (e.g. there's no Outcome filtering)
• The user can select multiple outcomes for filtering (for example Failed and 'Not run' together)
• Selecting the 'All' option will deselect all other outcome options
• A selected option is displayed with bold text to highlight that this filter is active
• The filter is only visible for the NUnit tree display (And hidden for Fixture/TestList display)
• The toolbar buttons are only enabled, if a project is loaded
• But the toolbar buttons are enabled during a test run

Technical point of view:
This PR includes a lot of changed files, but most of them are UI related. I hope it's possible to get an overview. Here are some aspects:

• I introduced a new UI interface IMultiSelection. That's similar to the existing interface ISelection, but supports the selection of multiple items.
• I introduced two new UI classes for the toolbar buttons: MultiCheckedToolStripButtonGroup and MultiCheckedToolStripButtonGroupWithDefaultButton. I'm not completly convinced by the class names myself, so I'm open for any suggestions - finding good names is always a challenge :-)
• The class MultiCheckedToolStripButtonGroup represents a group of Toolstrip buttons, several of which can be checked. Apart from that it's implementing the IViewElement interface like any other UI element.
• The class MultiCheckedToolStripButtonGroupWithDefaultButton derives from the MultiCheckedToolStripButtonGroup class by promoting one button to a DefaultButton, which resets the checked state of all other buttons if checked himself. (That's required if the 'All' outcome button is checked)
• I added an image resource for the first time to TestCentric (For the 'All' outcome button). I had the impression that I should add it to the resx file and to the image folder - at least that was my impression when looking to other images.
• I created the image myself - maybe it's not that creative ;-)
  But I wanted to express somehow visual that all Passed, Failed, 'not run' tests are included - so I simple add all colors (Green, Red, Grey).

[rowo360]

@CharliePoole:
May I kindly draw your attention to this next PR? Thanks a lot! :-)

[CharliePoole]

@rowo360 Sorry for the delay. I'm trying to build your branch now and getting an error in the Package target even though the CI worked. Are you able to do a local build -t Test -t Package?

[CharliePoole]

I cleared my local nuget caches and now it's OK.

[CharliePoole]

Nice work overall. I was able to figure out how to use it it generally does what it says it will do. I'm initially only reviewing the UI functionality and appearance, without looking too much at the code itself. You can expect further in-line comments later.

Changes I'd like to see...

1. For someone who isn't doing any filtering, I think the filtering menu bar is a bit obtrusive. Some options I can think of...

   • Have a Filter button in the existing main menu bar, with a dropdown menu attached. It should only be visible for the NUnit Tree. There's a standard icon for this.
   • Use a context menu on the tree display in the same way.
   • Have a Filters main menu.

2. The "All" button is a checkbox button. Each time you click it it turns filtering on and off. I think a "Clear" button, which merely turned off filters, would be clearer. :-)

3. If you turn on multiple checkboxes and then turn them all off, nothing is displayed. That seems like a bug, but it might go away after you do item 2.

I'll come back with code comments in a while.

[CharliePoole]

At first glance, this class seems unnecessary. You're putting functionality in the view that would normally be in the presenter in the MVP Pattern. The button that clears the others could just be a button. When the presenter detects that it has been clicked, it can clear all the selected items in the mulii-selection group. To do this, you would need to give the group a method to clear everything. I think that would make a good compromise as compared to having the presenter clear each button separately.

[rowo360]

Thanks for your comments and suggestions - that helps me to reconsider some decisions ;-)
Therefore I propose to adapt these points:

• I'll add a filter button to the main toolbar. That's a 'checkable' button to Show/Hide the filter toolbars. As usual we can store the state in the settings so that it's reapplied when reopening TestCentric the next time. And it's only enabled for the NUnit Tree Display.
• I'll eliminate the entire UI class you mentioned and move the functionality to a Presenter class.
• Clicking twice on the 'All' filter button (or twice on the 'Passed' button) will display nothing. And you are completely right, that's confusing for the user and should be improved.
  After considering the various options, I think it's best to remove the “All” button completely. We keep the 'Passed', 'Failed', 'Not run' buttons, which are all checked by default. The user can uncheck these buttons individually to filter the tree accordingly. And yes, if he unchecks all of them, we'll still display nothing - but I think this is easy to understand for the user.
  Overall I've the impression that the UI will be simplified with this step.
• Finally, a brief preview at the 'Clear filter' functionality: I plan to provide it with one of the next PRs soon. It will be a separate toolbar button which clears all active filters (outcome, text or category) all at once.

[CharliePoole]

Sounds good. The main toolbar button can make any and all filtering UI visible in the tree display. Initially it will just be categories, but will change as you add more filters. The main toolbar and presenter doesn't need to know exactly what filtering is possible.

[rowo360]

This commit addresses the first point: removing the 'All' button and removing the UI class.
It turns out that the original idea to remove the 'All' button reveals a gap: the user cannot show the inconclusive, skipped or ignored tests anymore once he started to use the filter. So I decided to add a new toolbar button 'Warning' which filters for all these states combined.

[rowo360]

This commit adds a Show/Hide filter button to the main toolbar. It's only enabled for the NUnit Display strategy.

[rowo360]

This is the next and currently last commit: I noticed that not all tree node icons have a transparent color set. This is not noticable in the tree view with his white background, but on the outcome toolbar buttons.
Here's a screenshot - note the white rectangles:

And after updating the images it looks like this:

Some tree icons were only present in jpg format which doesn't support transparent colors, so I converted them to png files. But independent from my changes I've the impression that the 'Classic' and 'Circle' icons looks somehow distorted.

[rowo360]

Oh, it's failing - I need to check it!

[rowo360]

That looks better now! :-)
Actually, I thought: “Adapt an image” - what could possibly go wrong? And took a shortcut...

@CharliePoole :
May I kindly ask you to look at my adaptations and decide if the requested changes are addressed? Thanks!

[CharliePoole]

This is extremely difficult to review with 53 files changed! No need to explain as I eventually figured out what you were dealing with. We'll work this one through but please try to keep to smaller commits in future, dealing with one issue at a time.

Please create issues for any separate problems you solved in this PR, icon transparency for sure but any other similar general issues. That way, when we do a release an entry for the bug fix will be generated automatically in the release notes. Otherwise, there is no record of what got fixed.

Be patient, for each new review, I download your branch, build it and exercise it. It's the only way I know to review GUI. For this one, I'lll probably comment one commit at a time but you won't see the comments till I finalize the review.

[CharliePoole]

I'm marking it as changes requested, although my suggestions are somewhat vague - even questions in some cases, so let's discuss. We may decide to leave some things as they are till we have another type of filter in the mix. That might make it clearer what we need to do.

[rowo360]

First of all, I'm sorry for the confusion about this large commit of all those images!
That was not a planned action, but more a spontaneous reaction as I noticed the behavior and considered that I can fix this quickly.
But that was not one of my best ideas and I totally agree that such a change is located better in a separate PR. :-)
Therefore I suggest that I rollback that images changes.

[CharliePoole]

Rolling back would make the final comparison cleaner, but isn't worth the extra effort. You already took the time to do it and I already took a bit of time to understand it. Best is to just create a new issue, which will be resolved by this PR, so that the reason for the change is visible in the release notes and future devs and users don't have to dig through our comments here in order to understand it.

I can imagine myself getting carried away in the same manner in your place, but ideally I would have made a note or even created an issue for changing the icons. If the PR were already created (as now) that issue would have to wait until it was merged. If it had not been created, I might just quickly merge a new PR to resolve it, then rebase this branch on top of it and do a forced push. Forced push on a branch that's not yet merged is OK but unfortunately GItHub deals badly with it if there is already a PR. I should write these "rules" down somewhere, I guess. :-)

[CharliePoole]

Great! Let's finish this up. I have an idea for some better ways to experiment with the UI before doing a PR. Let me think on it for a day.

[rowo360]

Regarding those image changes: I think I'd rather do it right this time and using the correct approach.
That means: do a rollback, create a separate issue and handle those changes with a separate PR as you proposed as the best approach.

Apart from this I'll adapt those code parts you mentioned and most important I'll remove the text from the filter toolbar buttons to reduce some space.

[CharliePoole]

Well, OK if you insist. But be sure it's a rollback, i.e. another commit to reverse the changes. As I explained, you can't just drop the commits and do a push -f safely once there is an active PR.

[rowo360]

I tried to address all requested changes. But either Github is tricking me or I missed one change request ?!?
Ultimately, however, the “Merge” option is still blocked.
@CharliePoole : Can you resolve it? Thanks!

[CharliePoole]

@rowo360 You are blocked because I requested changes and I have to re-review. GitHub doesn't know if your changes are satisfactory.

I see you did a forced push, which is what I asked you to avoid. If necessary, I'll go in merge it manually. That will be in the morning however. It's 3AM and I'm only briefly awake here. :-)

[CharliePoole]

@rowo360 I went ahead and bypassed the checks to merge this PR. I wanted to look more closely but I have a dental procedure to be done tomorrow so I thought I should get you unblocked. Hopefully the merge will succeed and build.