refactor(core): read tray icon only on desktop, refactor Context#6719
refactor(core): read tray icon only on desktop, refactor Context#6719lucasfernog merged 5 commits intonextfrom
Conversation
chippers
left a comment
chippers
left a comment
There was a problem hiding this comment.
There's 2 naming convention requests and 1 security-borderline issue. The is_ naming convention is a take it or leave it, but I think the with_ one is confusing.
Definitely fix the Default scope open regex however.
On a more of a "comment" note, we should probably decide on an architectural way to prevent so many cfgs mixed in with application logic. My first item to reach to would be to refactor concepts into Structs/Enums and use cfg inside those items to make them noops on cfg'd out platforms. Then the flow of stuff isn't broken up by cfg items all the time. This can apply to both the codebase and the codegen codebase.
core/tauri-codegen/src/context.rs
Outdated
| fn mobile(&self) -> bool { | ||
| matches!(self, Target::Android | Target::Ios) | ||
| } | ||
|
|
||
| fn desktop(&self) -> bool { | ||
| !self.mobile() | ||
| } |
There was a problem hiding this comment.
Not 100% sure we've been following this convention throughout the codebase, but since these aren't returning a "field" on the item but a property of it we may want to use the is_ prefix.
| fn mobile(&self) -> bool { | |
| matches!(self, Target::Android | Target::Ios) | |
| } | |
| fn desktop(&self) -> bool { | |
| !self.mobile() | |
| } | |
| fn is_mobile(&self) -> bool { | |
| matches!(self, Target::Android | Target::Ios) | |
| } | |
| fn is_desktop(&self) -> bool { | |
| !self.mobile() | |
| } |
core/tauri/src/scope/shell.rs
Outdated
|
|
||
| /// Shell scope configuration. | ||
| #[derive(Debug, Clone)] | ||
| #[derive(Debug, Default, Clone)] |
There was a problem hiding this comment.
This derive will have the open field set to None by default, which means that it is not matched through the default regex. It should probably match the default of ^((mailto:\w+)|(tel:\w+)|(https?://\w+)).+. We may want to adjust how we create it from the config so that we don't need to have the "default" filtering regex in more than 1 place, to reduce future duplication mistakes.
core/tauri/src/lib.rs
Outdated
| pub fn with_system_tray_icon(&mut self, icon: Icon) { | ||
| self.system_tray_icon.replace(icon); | ||
| } | ||
|
|
||
| /// Sets the app shell scope. | ||
| #[cfg(shell_scope)] | ||
| #[inline(always)] | ||
| pub fn with_shell_scope(&mut self, scope: scope::ShellScopeConfig) { |
There was a problem hiding this comment.
with_ prefixed methods are typically used for alternative constructors, not replacements of a single field. If there is a self being passed to a with_ function, then it's probably named wrong. I think set_ in this case works best since it's completely replacing a single item.
| pub fn with_system_tray_icon(&mut self, icon: Icon) { | |
| self.system_tray_icon.replace(icon); | |
| } | |
| /// Sets the app shell scope. | |
| #[cfg(shell_scope)] | |
| #[inline(always)] | |
| pub fn with_shell_scope(&mut self, scope: scope::ShellScopeConfig) { | |
| pub fn set_system_tray_icon(&mut self, icon: Icon) { | |
| self.system_tray_icon.replace(icon); | |
| } | |
| /// Sets the app shell scope. | |
| #[cfg(shell_scope)] | |
| #[inline(always)] | |
| pub fn set_shell_scope(&mut self, scope: scope::ShellScopeConfig) { |
2 participants
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Checklist
fix: remove a typo, closes #___, #___)Other information