[Trivial] Simplify member access #12293
Conversation
| @@ -30,7 +30,7 @@ public static void ShowFlyout(Control target, FlyoutBase flyout, IObservable<boo | |||
| { | |||
| condition = condition.CombineLatest( | |||
| window | |||
| .Select(window => window?.GetObservable(Window.IsActiveProperty) ?? Observable.Return(false)) | |||
| .Select(window => window?.GetObservable(WindowBase.IsActiveProperty) ?? Observable.Return(false)) | |||
There was a problem hiding this comment.
The change is correct, but this code is really weird because window hides another parameter named window in a wider scope. For me what would be correct is this:
| .Select(window => window?.GetObservable(WindowBase.IsActiveProperty) ?? Observable.Return(false)) | |
| .Select(x => x.GetObservable(WindowBase.IsActiveProperty)) |
Same line 20:
window
.Select(x => Observable.FromEventPattern<PixelPointEventArgs>(
handler => x.PositionChanged += handler,
handler => x.PositionChanged -= handler))
There was a problem hiding this comment.
I have no idea if this change is correct or not. @soosr can you check it please?
There was a problem hiding this comment.
@yahiheb if you revert this controversial change I will merge the rest. You can add comment to suspend if this causing warning at the compiler.
There was a problem hiding this comment.
How is this change controversial? It is not. I'm simply saying that the rest of the function is wrong as well. I approved the PR btw
There was a problem hiding this comment.
I don't think this is controversial as Clement said. In both cases the WindowBase.IsActiveProperty field is called.
There was a problem hiding this comment.
The suggested modifications here https://github.com/zkSNACKs/WalletWasabi/pull/12293/files#r1461175589 both make sense
There was a problem hiding this comment.
I applied Clement's suggestions: 17d72b7
There was a problem hiding this comment.
I have two suggestions for how to simplify the merging of these PRs with less risk in the future.
- First idea: if the PR is [Trivial] just format, typo, taking compiler suggestion, renaming, etc, we should not mix refactorings into it. Even if something is found to be useful it must be done in an upcoming PR.
- Second, if a question/comment pops up at any of the modifications the author of the PR facilitates the conversation by using
git blameat the specific line. This way we can reduce the number of ppl getting involved.
- Do not mix!
- Use
git blame
@yahiheb are you familiar with git blame?
| @@ -30,7 +30,7 @@ public static void ShowFlyout(Control target, FlyoutBase flyout, IObservable<boo | |||
| { | |||
| condition = condition.CombineLatest( | |||
| window | |||
| .Select(window => window?.GetObservable(Window.IsActiveProperty) ?? Observable.Return(false)) | |||
| .Select(window => window?.GetObservable(WindowBase.IsActiveProperty) ?? Observable.Return(false)) | |||
There was a problem hiding this comment.
I have no idea if this change is correct or not. @soosr can you check it please?
| @@ -30,7 +30,7 @@ public static void ShowFlyout(Control target, FlyoutBase flyout, IObservable<boo | |||
| { | |||
| condition = condition.CombineLatest( | |||
| window | |||
| .Select(window => window?.GetObservable(Window.IsActiveProperty) ?? Observable.Return(false)) | |||
| .Select(window => window?.GetObservable(WindowBase.IsActiveProperty) ?? Observable.Return(false)) | |||
There was a problem hiding this comment.
@yahiheb if you revert this controversial change I will merge the rest. You can add comment to suspend if this causing warning at the compiler.
|
@yahiheb What's up with this? |
As Clement has pointed out twice the change is correct. His comment was about the rest of the code, not what I have changed in this PR. And as I have stated the same |
|
I thought you would get @soosr or someone from the UI to check this line and clarify the issue that was brought up. Or we don't care with that? |
No description provided.