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

refactor!: simplify TrayIconEvent in JS by tagging it with type field #11121

Merged
merged 12 commits into from
Sep 26, 2024

Conversation

amrbashir
Copy link
Member

@amrbashir amrbashir commented Sep 25, 2024

@amrbashir amrbashir requested a review from a team as a code owner September 25, 2024 03:13
Copy link
Contributor

github-actions bot commented Sep 25, 2024

Package Changes Through ee90884

There are 10 changes which include tauri with prerelease, @tauri-apps/api with prerelease, tauri-bundler with prerelease, tauri-cli with prerelease, tauri-runtime-wry with prerelease, tauri-runtime with prerelease, tauri-utils with prerelease, @tauri-apps/cli with prerelease, tauri-build with prerelease, tauri-codegen with prerelease

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
@tauri-apps/api 2.0.0-rc.5 2.0.0-rc.6
tauri-utils 2.0.0-rc.12 2.0.0-rc.13
tauri-bundler 2.0.1-rc.13 2.0.1-rc.14
tauri-runtime 2.0.0-rc.12 2.0.0-rc.13
tauri-runtime-wry 2.0.0-rc.13 2.0.0-rc.14
tauri-codegen 2.0.0-rc.12 2.0.0-rc.13
tauri-macros 2.0.0-rc.11 2.0.0-rc.12
tauri-plugin 2.0.0-rc.12 2.0.0-rc.13
tauri-build 2.0.0-rc.12 2.0.0-rc.13
tauri 2.0.0-rc.15 2.0.0-rc.16
@tauri-apps/cli 2.0.0-rc.16 2.0.0-rc.17
tauri-cli 2.0.0-rc.16 2.0.0-rc.17

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

packages/api/src/tray.ts Outdated Show resolved Hide resolved
@Legend-Master
Copy link
Contributor

Legend-Master commented Sep 25, 2024

diff --git a/packages/api/src/tray.ts b/packages/api/src/tray.ts
index 45820fe4d..b662fe8ab 100644
--- a/packages/api/src/tray.ts
+++ b/packages/api/src/tray.ts
@@ -16,15 +16,7 @@ export type TrayIconEventType =
   | 'Move'
   | 'Leave'
 
-/**
- * Describes a tray icon event.
- *
- * #### Platform-specific:
- *
- * - **Linux**: Unsupported. The event is not emitted even though the icon is shown,
- * the icon will still show a context menu on right click.
- */
-export type TrayIconEvent<T extends TrayIconEventType = TrayIconEventType> = {
+export type BaseTrayIconEvent<T extends TrayIconEventType> = {
   /** The tray icon event type */
   type: T
   /** Id of the tray icon which triggered this event. */
@@ -36,18 +28,52 @@ export type TrayIconEvent<T extends TrayIconEventType = TrayIconEventType> = {
     position: PhysicalPosition
     size: PhysicalSize
   }
-} & (T extends 'Click' | 'DoubleClick'
-  ? {
-      /** Mouse button that triggered this event. */
-      button: MouseButton
-    }
-  : never) &
-  (T extends 'Click'
-    ? {
+}
+
+export type TrayIconClickEvent = {
+  /** Mouse button that triggered this event. */
+  button: MouseButton
+}
+
+/**
+ * Describes a tray icon event.
+ *
+ * #### Platform-specific:
+ *
+ * - **Linux**: Unsupported. The event is not emitted even though the icon is shown,
+ * the icon will still show a context menu on right click.
+ */
+export type TrayIconEvent =
+  | (BaseTrayIconEvent<'Click'> &
+      TrayIconClickEvent & {
         /** Mouse button state when this event was triggered. */
         buttonState: MouseButtonState
+      })
+  | (BaseTrayIconEvent<'DoubleClick'> & TrayIconClickEvent)
+  | BaseTrayIconEvent<'Enter'>
+  | BaseTrayIconEvent<'Move'>
+  | BaseTrayIconEvent<'Leave'>
+
+type TrayIconEventInternal = Omit<TrayIconEvent, 'rect' | 'position'> & {
+  position: {
+    x: number
+    y: number
+  }
+  rect: {
+    position: {
+      Physical: {
+        x: number
+        y: number
       }
-    : never)
+    }
+    size: {
+      Physical: {
+        width: number
+        height: number
+      }
+    }
+  }
+}
 
 /**
  * Tray icon types and utilities.
@@ -166,37 +192,24 @@ export class TrayIcon extends Resource {
       options.icon = transformImage(options.icon)
     }
 
-    const handler = new Channel<TrayIconEvent>()
+    const handler = new Channel<TrayIconEventInternal>()
     if (options?.action) {
       const action = options.action
       handler.onmessage = (e) => {
-        if (
-          'Physical' in e.rect.position &&
-          e.rect.position.Physical &&
-          typeof e.rect.position.Physical === 'object' &&
-          'x' in e.rect.position.Physical &&
-          'y' in e.rect.position.Physical
-        ) {
-          e.rect.position = new PhysicalPosition(
-            e.rect.position.Physical.x as number,
-            e.rect.position.Physical.y as number
-          )
-        }
-        if (
-          'Physical' in e.rect.size &&
-          e.rect.size.Physical &&
-          typeof e.rect.size.Physical === 'object' &&
-          'width' in e.rect.size.Physical &&
-          'height' in e.rect.size.Physical
-        ) {
-          e.rect.size = new PhysicalSize(
-            e.rect.size.Physical.width as number,
-            e.rect.size.Physical.height as number
+        // e.position = new PhysicalPosition(e.position.x, e.position.y)
+        const out = e as unknown as TrayIconEvent
+        out.position = new PhysicalPosition(e.position.x, e.position.y)
+        out.rect = {
+          position: new PhysicalPosition(
+            e.rect.position.Physical.x,
+            e.rect.position.Physical.y
+          ),
+          size: new PhysicalSize(
+            e.rect.size.Physical.width,
+            e.rect.size.Physical.height
           )
         }
-        e.position = new PhysicalPosition(e.position.x, e.position.y)
-
-        action(e)
+        action(out)
       }
       delete options.action
     }

@amrbashir I think something like this would be better

@Legend-Master
Copy link
Contributor

Legend-Master commented Sep 25, 2024

If you like the idea of typing the internal event as well, here's the updated diff

diff --git a/packages/api/src/tray.ts b/packages/api/src/tray.ts
index 040117a09..118928dd2 100644
--- a/packages/api/src/tray.ts
+++ b/packages/api/src/tray.ts
@@ -52,6 +52,27 @@ export type TrayIconEvent =
   | TrayIconEventBase<'Move'>
   | TrayIconEventBase<'Leave'>
 
+type TrayIconEventInternal = Omit<TrayIconEvent, 'rect' | 'position'> & {
+  position: {
+    x: number
+    y: number
+  }
+  rect: {
+    position: {
+      Physical: {
+        x: number
+        y: number
+      }
+    }
+    size: {
+      Physical: {
+        width: number
+        height: number
+      }
+    }
+  }
+}
+
 /**
  * Tray icon types and utilities.
  *
@@ -169,37 +190,23 @@ export class TrayIcon extends Resource {
       options.icon = transformImage(options.icon)
     }
 
-    const handler = new Channel<TrayIconEvent>()
+    const handler = new Channel<TrayIconEventInternal>()
     if (options?.action) {
       const action = options.action
       handler.onmessage = (e) => {
-        if (
-          'Physical' in e.rect.position &&
-          e.rect.position.Physical &&
-          typeof e.rect.position.Physical === 'object' &&
-          'x' in e.rect.position.Physical &&
-          'y' in e.rect.position.Physical
-        ) {
-          e.rect.position = new PhysicalPosition(
-            e.rect.position.Physical.x as number,
-            e.rect.position.Physical.y as number
+        const out = e as unknown as TrayIconEvent
+        out.position = new PhysicalPosition(e.position.x, e.position.y)
+        out.rect = {
+          position: new PhysicalPosition(
+            e.rect.position.Physical.x,
+            e.rect.position.Physical.y
+          ),
+          size: new PhysicalSize(
+            e.rect.size.Physical.width,
+            e.rect.size.Physical.height
           )
         }
-        if (
-          'Physical' in e.rect.size &&
-          e.rect.size.Physical &&
-          typeof e.rect.size.Physical === 'object' &&
-          'width' in e.rect.size.Physical &&
-          'height' in e.rect.size.Physical
-        ) {
-          e.rect.size = new PhysicalSize(
-            e.rect.size.Physical.width as number,
-            e.rect.size.Physical.height as number
-          )
-        }
-        e.position = new PhysicalPosition(e.position.x, e.position.y)
-
-        action(e)
+        action(out)
       }
       delete options.action
     }

@amrbashir
Copy link
Member Author

narrowing using JS checks is better here IMO since it ensures we never construct invalid PhysicalPosition values when we don't receive any from Rust. I think I might need to add checks for Logical as well

@Legend-Master
Copy link
Contributor

Currently it's guaranteed to be PhysicalPosition but it might change depending on the tray-icon side, maybe we type two variants, and then do the conversion, the current way of checking is quite hard to understand and error prone to be honest

@Legend-Master
Copy link
Contributor

Legend-Master commented Sep 25, 2024

I honestly feel like it's probably easier to use a custom Rect or tray-icon's Rect in the rust side than doing it in the js side

@amrbashir
Copy link
Member Author

Currently it's guaranteed to be PhysicalPosition but it might change depending on the tray-icon side, maybe we type two variants, and then do the conversion, the current way of checking is quite hard to understand and error prone to be honest

I will add checks for Logical variants which should cover the case if the position type changes

As for the confusing part, I don't think there is a way around that, we still need to check whether Physical or Logical property exist or not, I will add some comments to explain why we doing this

I honestly feel like it's probably easier to use a custom Rect or tray-icon's Rect in the rust side than doing it in the js side

We need to create an instance of PhysicalPosition or LogicalPosition so we still need to do checks and create the instances on the JS side

@amrbashir
Copy link
Member Author

I have added checks for Logical and incorporated your suggestions for internal type

@Legend-Master
Copy link
Contributor

Legend-Master commented Sep 26, 2024

We need to create an instance of PhysicalPosition or LogicalPosition so we still need to do checks and create the instances on the JS side

We will still need to create the instance but we won't need to check LogicalPosition

I don't quite like this union type to be honest, this forces the user to check whether this is a physical one or a logical one, and doesn't really match the fact that we return PhysicalPosition and PhysicalSize on everything else

This is also hard to convert in the js side, getting the dpi requires an async ipc call, and we don't even have a getter for getting the dpi at position yet, we only have one for the windows

@amrbashir
Copy link
Member Author

good point, changed that and enforced the deserialization always matches on the Rust side

packages/api/src/tray.ts Outdated Show resolved Hide resolved
crates/tauri/src/tray/mod.rs Outdated Show resolved Hide resolved
amrbashir and others added 5 commits September 26, 2024 05:27
Co-authored-by: Tony <68118705+Legend-Master@users.noreply.github.com>
Co-authored-by: Tony <68118705+Legend-Master@users.noreply.github.com>
@amrbashir amrbashir merged commit 0b44959 into dev Sep 26, 2024
24 of 25 checks passed
@amrbashir amrbashir deleted the refactor/tray-icon-event-tag-type branch September 26, 2024 03:12
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.

2 participants