Pane Navigation & UIController #728
Conversation
| @@ -28,7 +28,7 @@ public PullRequestListView() | |||
|
|
|||
| OpenPR = new RelayCommand(x => | |||
| { | |||
| NotifyOpen(new ViewWithData { Data = x }); | |||
| NotifyOpen(new ViewWithData(UIControllerFlow.PullRequests) { ViewType = UIViewType.PRDetail, Data = x}); | |||
There was a problem hiding this comment.
Based on the unit tests I believe this is required.
| mainFlow); | ||
|
|
||
| throw new ArgumentException(message); | ||
| } |
There was a problem hiding this comment.
I think it's better to throw an error in this situation than Assert().
| Assert.True(uiController.IsStopped); | ||
| Assert.True(success.HasValue); | ||
| Assert.True(success); |
There was a problem hiding this comment.
Assert.True(success); bothers me here. If the last action performed was Cancel on the clone flow, shouldn't this value be False?
There was a problem hiding this comment.
iirc, success is false if the flow failed at the end without user intervention. The user cancelling a flow would not be a failure of the flow itself, merely an early exit. And yes, it doesn't feel very logical. I need to revisit some of the thoughts on this one...
| Assert.True(uiController.IsStopped); | ||
| Assert.True(success.HasValue); | ||
| Assert.True(success); |
There was a problem hiding this comment.
Same Assert.True(success); issue here.
|
I think the problem is due to the Next Trigger attempting to redirect to PRDetail which is a TriggerWithParam. My original fix attempted to intercept the Next Trigger in that scenario and issue a PRDetail TriggerWithParam, that works in a few scenarios, but it would require permitting a PRDetail TriggerWithParam everywhere a Next Trigger is specified. I cleaned up, uniformed and commented the tests in In |
Also temporarily expanded some compressed lambdas for easier debugging
|
After not thinking about it for a day, I think one solution could be to make every trigger a TriggerWithParam. Even if a param is not used/required; that would allow the Next operation to work regardless if the "Next" state needs a param or not. |
|
To be honest, I think UIController is taking on too much responsibility and shouldn't be handling all this work, which is manifesting itself in all these problems. It's meant to be a state machine handling logic for ui flows, which really doesn't work well when forced to jump to arbitrary points in the middle of a particular flow. |
|
Thanks for the attempt @StanleyGoldman, but we decided to rewrite the navigation controller in #752 so closing this. |
Attempting to resolve #628
In
UIControllerWhenJump()usesFire()to change states,Fire()does not correctly determine if the requested state inrequestedTargetrequires an argument.