Skip to content

Conversation

@Coloryr
Copy link
Contributor

@Coloryr Coloryr commented Mar 14, 2025

image
image

@SKProCH SKProCH self-requested a review March 14, 2025 09:01
@SKProCH
Copy link
Member

SKProCH commented Mar 16, 2025

I appreciate this work, the reason I haven't done it myself is because I don't know how it should be designed as a feature.

I need to spend a little more time thinking about this implementation.

Copy link
Member

@SKProCH SKProCH left a comment

Choose a reason for hiding this comment

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

Overall works fine, but i think there is need for proper handling of disabling of multiple dialogs. E.g. if i open 2 dialogs, i should be able to interact only with a top one (second one should be non interactable).
Close command also always closes the top dialog (but if other dialogs will be non interactable, then this is not a problem).

And probably we should move multidialog logic to separate control (since currently at least IsOpen would works weirdly), what do you think? Btw, after looking at this PR i kinda want to rewrite dialoghost almost from scratch, but i think what i don't have enough time

@SKProCH
Copy link
Member

SKProCH commented Mar 30, 2025

CurrentSession should be IReadOnlyList.
Also, you dont actually change anything about making non top dialogs non interactable.

E.g. if i open 2 dialogs, i should be able to interact only with a top one (second one should be non interactable).
Close command also always closes the top dialog (but if other dialogs will be non interactable, then this is not a problem).

Is it done intentionally, or you address this in a future, or i should do it myself?

@Coloryr
Copy link
Contributor Author

Coloryr commented Mar 31, 2025

CurrentSession should be IReadOnlyList. Also, you dont actually change anything about making non top dialogs non interactable.

E.g. if i open 2 dialogs, i should be able to interact only with a top one (second one should be non interactable).
Close command also always closes the top dialog (but if other dialogs will be non interactable, then this is not a problem).

Is it done intentionally, or you address this in a future, or i should do it myself?

CurrentSession would be better with the separate property, because CloseCommand only closes the current view, and turn next top view.
DisposeList It is used to unbind some bindings. When a top view closed, its should be canceled binding itself, or it can be move to pop view
I think it's better for you to complete it than for me

@Coloryr
Copy link
Contributor Author

Coloryr commented Nov 7, 2025

Hei, this is still going?

@SKProCH
Copy link
Member

SKProCH commented Nov 7, 2025

Yeah, I need to find some time for that, sorry.

@Coloryr
Copy link
Contributor Author

Coloryr commented Nov 13, 2025

You can tell me your idea, and I can finish it.
I need this feature on my next version program.

@Coloryr
Copy link
Contributor Author

Coloryr commented Nov 14, 2025

It's finish, could you check it?

@Coloryr Coloryr requested a review from SKProCH November 14, 2025 01:15
@SKProCH
Copy link
Member

SKProCH commented Nov 14, 2025

I'll take a look today

Copy link
Member

@SKProCH SKProCH left a comment

Choose a reason for hiding this comment

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

Overall fine

@SKProCH
Copy link
Member

SKProCH commented Nov 14, 2025

I remembered the reason I abandoned this PR earlier.
I wanted to think through how it should work (considering, for example, that we have a DialogContent property, which in the original version was always set to the contents of the currently open dialog, and the current contents could be controlled this way), but couldn't think of anything.
I still don't really understand the use case for this (by the way, why do you need this feature?), since all non-top-level dialogs would simply be overlaid by the last one, i.e., it probably wouldn't be very usable.
But honestly, I don't see how this could be designed any better, so let's have it like this.

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