Skip to content

feat(window-state): add filter callback for excluding windows from tracking #2330

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Jan 18, 2025
Merged

feat(window-state): add filter callback for excluding windows from tracking #2330

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2847f11
feat(window-state): Add glob pattern support to denylist for flexible…
thewh1teagle Jan 16, 2025
c8e84f6
changefile
thewh1teagle Jan 16, 2025
4aa8a51
refactor(window-state): Simplify denylist handling and error manageme…
thewh1teagle Jan 17, 2025
ff66ed3
feat: add with_denylist_glob
thewh1teagle Jan 17, 2025
1368d4a
feat(window-state): Add filter callback for excluding windows from tr…
thewh1teagle Jan 17, 2025
eca5293
refactor(window-state): Remove unused Glob error variant from Error enum
thewh1teagle Jan 17, 2025
6d8d565
refactor(window-state): Change filter_callback from Arc<Mutex<FilterC…
thewh1teagle Jan 17, 2025
ead110d
refactor(window-state): Remove glob dependency from Cargo.toml and Ca…
thewh1teagle Jan 17, 2025
20bdfa5
feat(window-state): Introduce filter callback for dynamic window excl…
thewh1teagle Jan 17, 2025
ae08144
Update .changes/window-state-filter-callback.md
amrbashir Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/window-state-glob-pattern-denylist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
window-state: patch
window-state-js: patch
---

Introduces the ability to use glob patterns in the denylist for windows that shouldn't be tracked and managed. This allows for more flexible window matching, such as ignoring multiple windows that fit a certain naming pattern.
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions plugins/window-state/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,5 @@ serde_json = { workspace = true }
tauri = { workspace = true }
log = { workspace = true }
thiserror = { workspace = true }
glob = { workspace = true }
bitflags = "2"
24 changes: 17 additions & 7 deletions plugins/window-state/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ pub enum Error {
Tauri(#[from] tauri::Error),
#[error(transparent)]
SerdeJson(#[from] serde_json::Error),
#[error(transparent)]
Glob(#[from] glob::PatternError),
}

pub type Result<T> = std::result::Result<T, Error>;
Expand Down Expand Up @@ -319,7 +321,7 @@ impl<R: Runtime> WindowExtInternal for Window<R> {

#[derive(Default)]
pub struct Builder {
denylist: HashSet<String>,
denylist: Vec<glob::Pattern>,
skip_initial_state: HashSet<String>,
state_flags: StateFlags,
map_label: Option<Box<LabelMapperFn>>,
Expand All @@ -344,10 +346,16 @@ impl Builder {
}

/// Sets a list of windows that shouldn't be tracked and managed by this plugin
/// for example splash screen windows.
pub fn with_denylist(mut self, denylist: &[&str]) -> Self {
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> {
Copy link
Member

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

Copy link
Contributor Author

@thewh1teagle thewh1teagle Jan 17, 2025 •
edited
Loading

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)

Copy link
Contributor Author

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
}

Copy link
Member

@amrbashir amrbashir Jan 17, 2025 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why so?

Copy link
Contributor Author

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.

Copy link
Contributor

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()

Copy link
Contributor Author

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()

denylist.sort();

let mut denylist_patterns = Vec::new();
for pattern in denylist {
denylist_patterns.push(glob::Pattern::new(pattern)?);
}
self.denylist = denylist_patterns;
Ok(self)
}

/// Adds the given window label to a list of windows to skip initial state restore.
Expand Down Expand Up @@ -413,8 +421,10 @@ impl Builder {
.map(|map| map(window.label()))
.unwrap_or_else(|| window.label());

if self.denylist.contains(label) {
return;
for pattern in &self.denylist {
if pattern.matches(label) {
return;
}
}

if !self.skip_initial_state.contains(label) {
Expand Down
Loading