-
Notifications
You must be signed in to change notification settings - Fork 300
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
feat(window-state): add filter callback for excluding windows from tracking #2330
feat(window-state): add filter callback for excluding windows from tracking #2330
Conversation
… window management
fix(window-state): Remove unnecessary reference in denylist pattern processing chore(changes): Update window-state and window-state-js versioning for glob pattern support docs(window-state): Update glob pattern denylist documentation for clarity
ad0a181
to
c8e84f6
Compare
Package Changes Through ae08144There are 6 changes which include haptics with patch, haptics-js with patch, geolocation with patch, geolocation-js with patch, window-state with patch, window-state-js with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
plugins/window-state/src/lib.rs
Outdated
self.denylist = denylist.iter().map(|l| l.to_string()).collect(); | ||
self | ||
/// For example, splash screen windows. It also supports glob patterns for flexible window matching. | ||
pub fn with_denylist(mut self, denylist: &mut [&str]) -> Result<Self> { |
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.
this is a breaking change, let's add the globbing into a different function or simply ignore the glob errors and store and match against it as is
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.
this is a breaking change, let's add the globbing into a different function or simply ignore the glob errors and store and match against it as is
I'll change into
denylist_patterns.push(glob::Pattern::new(pattern).expect("Failed to parse glob pattern"));
Should I also change back the &mut [&str] denylist into &[&str]? (not sure how then sort it nicely)
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.
Maybe
pub fn with_denylist(mut self, denylist: &[&str]) -> Self {
let mut denylist = denylist.to_vec();
denylist.sort();
let mut denylist_patterns = Vec::new();
for pattern in denylist {
denylist_patterns.push(glob::Pattern::new(pattern).expect("Failed to parse glob pattern"));
}
self.denylist = denylist_patterns;
self
}
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.
Why do you need to sort?
Also please don't call .expect()
, that's just a breaking change in disguise.
Instead, make two deny lists, one for globs and one for normal labels. if the pattern is built successfully then append to patterns list, otherwise append to normal list.
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.
why do you need to sort?
In second thought, we don't need.
also please don't call
.expect()
, that's just a breaking change in disguise.
What do you suggest? If returning a Result
isn't an option, I'm not sure what else to do.
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.
To be honest, I feel like if we're adding it in this way, a function might work better, the error handling here just feels wrong to me
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.
why so?
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.
To be honest, I feel like if we're adding it in this way, a function might work better, the error handling here just feels wrong to me
The denylist patterns will run during every reload of the app in development. The chances of the production build having an error with it are small.
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 was thinking they would re-use the same function but then I realized they can't as that will be a breaking change
If we're adding in another function just for the globs, I feel like it's enough reason to let the user provide a function instead, this will also allow some more advanced use cases like only skip for next window creation as well
Also in most cases, I believe a check like window_label.starts_with("some-name")
would probably be enough
Another thing is I find functions returning results like this is quite confusing in general, I always wondered about why would this function ever fail? It's an extra mental burden
I'm not against this though, just find myself preferring a function more
tauri_plugin_window_state::Builder::new() .with_denylist_glob(&["window-*"]) .unwrap() .build()
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.
If we're adding in another function just for the globs, I feel like it's enough reason to let the user provide a function instead, this will also allow some more advanced use cases like only skip for next window creation as well
sounds better
How about
tauri_plugin_window_state::Builder::new()
.with_filter(|label| true) // return true to save the state
.build()
…nt in with_denylist method
Just a suggestion, instead of expending on the pattern, what about we allow an user provided function to customize this? Maybe it's fine to just expand with glob patterns in this use case though |
We have a similar function |
…allbackFn>> to Box<FilterCallbackFn>
Add filter callback for excluding windows from tracking for window state plugin
Usage:
Resolve