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

Fixes #3522. Adds IDesignable: Ability for Views to load design-time/demo data #3568

Closed
wants to merge 16 commits into from

Conversation

tig
Copy link
Collaborator

@tig tig commented Jun 26, 2024

Replaced by #3575

I don't know what rebase error I keep making, but I continue to eff my branches up. Arrrrg.

@tig tig self-assigned this Jun 26, 2024
@tig tig marked this pull request as ready for review June 26, 2024 18:46
@tig tig requested review from BDisp and tznind June 26, 2024 18:47
Copy link
Collaborator

@BDisp BDisp left a comment

Choose a reason for hiding this comment

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

LGTM. But I would like @tznind take a look on this. Thanks.

Copy link
Collaborator

@tznind tznind left a comment

Choose a reason for hiding this comment

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

So this function updates the state of the View to an 'example' state?

This makes sense and means some code can come out of TGD ViewFactory.

Makes sense for the view tester too.

I like it!

Terminal.Gui/View/ISupportsDesignMode.cs Outdated Show resolved Hide resolved
@tig tig changed the title Fixes #3522. Adds ability for Views to have "design mode" Fixes #3522. Adds IDisignable: Ability for Views to load design-time/demo data Jun 26, 2024
@dodexahedron
Copy link
Collaborator

Neat. I'll take a look at this one either later tonight or some time tomorrow.

@tig tig changed the title Fixes #3522. Adds IDisignable: Ability for Views to load design-time/demo data Fixes #3522. Adds IDesignable: Ability for Views to load design-time/demo data Jun 27, 2024
@dodexahedron
Copy link
Collaborator

Getting to this in a few minutes, once I go through the last few items at the top of my inbox.

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me.

But I made a couple of suggestions with code for some design-time and run-time improvements.

Terminal.Gui/View/IDesignable.cs Outdated Show resolved Hide resolved
Terminal.Gui/Views/Button.cs Outdated Show resolved Hide resolved
@tig
Copy link
Collaborator Author

tig commented Jun 27, 2024

Regarding other possible things to add to this interface: Not gonna do it until actually needed. First rule of API design is to not expose something publicly until there are at least 3 known use cases.

@tig
Copy link
Collaborator Author

tig commented Jun 27, 2024

Ok, how's this?

@dodexahedron
Copy link
Collaborator

I think your original naming was better. Enable is kinda confusing since that's now a member of the View. I wouldn't expect SomeView.Enable() to load demo data.

@dodexahedron
Copy link
Collaborator

Regarding other possible things to add to this interface: Not gonna do it until actually needed. First rule of API design is to not expose something publicly until there are at least 3 known use cases.

Yep, agree there.

In any case, pretty much everything needed is already in the View class and derivatives anyway. Maybe we should take a refinement pass in beta or something to go through and annotate things around the library with appropriate/useful attributes like that?

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

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

Almost there.

I think good to go with these additional little tweaks.

Terminal.Gui/Views/Menu/MenuBar.cs Outdated Show resolved Hide resolved
UICatalog/Scenarios/AllViewsTester.cs Outdated Show resolved Hide resolved
@tig
Copy link
Collaborator Author

tig commented Jun 28, 2024

I think your original naming was better. Enable is kinda confusing since that's now a member of the View. I wouldn't expect SomeView.Enable() to load demo data.

Now EnableForDesign

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

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

I don't really have a lot to add beyond what we talked about in the issue already.

All I put in here was a suggestion/remindr about CombinatorialData for XUnit. Unimportant.

[Theory]
// 0123456789
// " C 0 A "
[InlineData (-1, 0)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about, instead of all these inlines:

    [Theory]
    [CombinatorialData]
    [InlineData (-1, 0)]
    [InlineData (9, 0)]
    [AutoInitShutdown]
    public void MouseClick_Fires_Accept ([CombinatorialRange (0, 8)] int x, [CombinatorialValues (1)] int expectedAccept)

Or similar.

InlineData can still be used for one-off cases, as shown, and are explicit cases beyond the "combinatorial" cases, but do get de-duplicated if you accidentally specify any dupes.

Don't bother if you don't want to, though.

There are other things about the tests that are more important, but that's so far down my list right now. 😅

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.

None yet

4 participants