-
-
Notifications
You must be signed in to change notification settings - Fork 446
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 #481 - Keep selected filter option between application restarts. #503
Conversation
}; | ||
} | ||
|
||
public int SelectedDefaultFilterIndex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to remove these and revert back, it's public because I was going to use it for the binding (which didn't stop the issue), but I figured I'd leave it in case that's actually the better way
Loaded += (s, e) => { SelectCurrentFilter(); }; | ||
Loaded += (s, e) => { | ||
SelectCurrentFilter(); | ||
EverythingSearch.Instance.PropertyChanged += OnCurrentFilterChanged; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EverythinhSearch.Instance.PropertyChanged
could fire prior to the element loading
@@ -47,6 +63,11 @@ private void OnComboBoxItemSelected(object sender, SelectionChangedEventArgs e) | |||
if (ComboBox.SelectedIndex < 0) | |||
return; | |||
|
|||
if (!ComboBox.IsFocused && !ComboBox.IsMouseOver) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During testing this code was reached (and toggles selection) when an item is selected from the dropdown list, there wasn't another reliable property I could find that would indicate that the window is actually open (during the Show()
triggering of this event, even though the window wasn't yet visible, both isVisible
and isHitBoxVisible
were true
.
Hi @zeyus, thanks a lot for your contribution. I remember trying this briefly and running into similar problems. The solution feels a little hacky, but then again I also don't know a better solution. One thing that comes to mind: With the focus checks on the |
No probs @srwi, least I can do seeing as I'm enjoying using EverythingToolbar :) I agree it feels a little hacky, although this does specifically check that the selected item change is due to a user interaction rather than the initial population / sync with binding, so conceptually it makes sense. What I don't understand is that there isn't an obvious way to prevent population/syncing from firing the events (or to bind the handlers after all items are loaded). Maybe it requires way more Just confirmed keyboard shortcuts are working as expected (because the tab/combo elements have |
You're right. I think it is fine. Especially since it's well contained within the FilterSelector control.
Probably the correct way would be to bind the currently selected filter to both In any case, I really like your solution and it works quite well for me, so I'll merge the PR. Thanks again for your contribution! |
This PR fixes #481 so the selected filter is saved on selection change, and updates the tab and combo to only update if the element is under the mouse cursor or focused.
I implemented it this way because I'm not familiar with
C#
and couldn't figure out why onShow()
it would fire the SelectionChanged event...In the code there should be nothing triggering it (it kept defaulting back toAll
even though when I was debugging, theSettings.Default.lastFilter
showed the correctly selected filter.Anyway, if you have a suggestion on how to avoid that issue and maybe bind the
SelectedIndex
directly (I tried that and it still fired onShow()
) let me know, and I will update the PR, otherwise, this works. :)