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 #481 - Keep selected filter option between application restarts. #503

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

zeyus
Copy link
Contributor

@zeyus zeyus commented Jun 10, 2024

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 on Show() it would fire the SelectionChanged event...In the code there should be nothing triggering it (it kept defaulting back to All even though when I was debugging, the Settings.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 on Show()) let me know, and I will update the PR, otherwise, this works. :)

};
}

public int SelectedDefaultFilterIndex
Copy link
Contributor Author

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;
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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.

@srwi
Copy link
Owner

srwi commented Jun 11, 2024

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 ComboBox and TabControl, is it still possible to select filters using the keyboard shortcuts Tab/Shift+Tab?

@zeyus
Copy link
Contributor Author

zeyus commented Jun 12, 2024

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 ComboBox and TabControl, is it still possible to select filters using the keyboard shortcuts Tab/Shift+Tab?

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 C# foo than I have haha.

Just confirmed keyboard shortcuts are working as expected (because the tab/combo elements have isFocused == True when tabbing to them).

tab-shift-tab

@srwi
Copy link
Owner

srwi commented Jun 13, 2024

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.

You're right. I think it is fine. Especially since it's well contained within the FilterSelector control.

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 C# foo than I have haha.

Probably the correct way would be to bind the currently selected filter to both SelectedIndexs with a value converter rather than listening to the property changing, but then I don't really know whether it would help at all with the problem you're describing.

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!

@srwi srwi merged commit 6abbd88 into srwi:master Jun 13, 2024
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.

Remember selected filter after restart
2 participants