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

Migrate to tauri v2 #18

Merged
merged 6 commits into from
Nov 6, 2024
Merged

Migrate to tauri v2 #18

merged 6 commits into from
Nov 6, 2024

Conversation

Pinguin2001
Copy link
Contributor

This commit migrates the project to the latest tauri v2.

As I currently do not have access to a device running a relatively new version of mac os currently, functionality on macOS should be validated first before merging this pr

@Abdenasser Abdenasser self-requested a review November 6, 2024 16:09
@@ -1,20 +1,12 @@
#![cfg_attr(not(debug_assertions), windows_subsystem = "windows")]

use std::collections::HashMap;
Copy link
Owner

@Abdenasser Abdenasser Nov 6, 2024

Choose a reason for hiding this comment

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

what formatter are we using? this would be hard for me to carefully review, can we only change code related to migration and keep the same formatting as before?

Copy link
Owner

Choose a reason for hiding this comment

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

otherwise everything else looks good to me, thank you 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been formatted using the default rust lang support plugin for vscode.

I have reverted my changes, please take a look at it again

tauri-plugin-process = "2"
tauri-plugin-fs = "2"
tauri-plugin-dialog = "2"
tauri-plugin-clipboard-manager = "2.0.2"
Copy link
Owner

Choose a reason for hiding this comment

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

@Pinguin2001 are these some V2 requirements ?
tauri-plugin-os = "2"
tauri-plugin-http = "2"
tauri-plugin-notification = "2"
tauri-plugin-process = "2"
tauri-plugin-clipboard-manager = "2.0.2"
tauri-plugin-global-shortcut = "2"

@@ -25,3 +33,6 @@ lto = "fat" # More aggressive link-time optimization
opt-level = 3 # Optimize for maximum performance
strip = true # Remove debug symbols
incremental = false # Disable incremental compilation

[target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies]
tauri-plugin-global-shortcut = "2"
Copy link
Owner

Choose a reason for hiding this comment

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

this as well?

@@ -0,0 +1,11 @@
{
"identifier": "desktop-capability",
Copy link
Owner

Choose a reason for hiding this comment

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

why we need this one? while we don't expose any command called desktop-capability.

@@ -0,0 +1,87 @@
{
"identifier": "migrated",
Copy link
Owner

Choose a reason for hiding this comment

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

is there a command called migrated??

Copy link

Choose a reason for hiding this comment

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

Capabilities are a new feature that granularly enables and constrains the core exposure to the application front-end. They are essentially a set of permissions mapped to application windows. The migrated.js represents the list of capabilities, extracted during the automatic migration step, used in the app.

Copy link
Owner

@Abdenasser Abdenasser Nov 6, 2024

Choose a reason for hiding this comment

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

yeah they are new, but not just random.. the identifiers would most of the time refer to commands we expose in invoke handler.. I remember using the windows identifier as "windows"

Copy link
Owner

Choose a reason for hiding this comment

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

not because it was added by the migration script that this is the way it should be.. try creating an app with V2 directly.

.plugin(tauri_plugin_notification::init())
.plugin(tauri_plugin_http::init())
.plugin(tauri_plugin_shell::init())
.plugin(tauri_plugin_os::init())
Copy link
Owner

Choose a reason for hiding this comment

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

why adding this full list of plugins?

Copy link

Choose a reason for hiding this comment

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

In Tauri 1.0, many plugins were included by default potentially including unnecessary functionality. With Tauri 2.0, the framework has moved towards a more modular approach, where you explicitly add only the plugins you need.

Copy link
Owner

Choose a reason for hiding this comment

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

interesting, but still I'm not 100% sure they are required.. worth exploring a bit more

Copy link

Choose a reason for hiding this comment

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

I agree. The new version of Tauri brings a lot to the table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting, but still I'm not 100% sure they are required.. worth exploring a bit more

Not all of them are required, I'm exploring and testing right now.

@Pinguin2001
Copy link
Contributor Author

These changes were automatically added by the tauri v2 migrator.

Changes have been reverted as requested, in case you need a plugin from tauri later, it has to be added manually.

@Pinguin2001
Copy link
Contributor Author

@Abdenasser Please take a look at it again, I have tested all functionality and everything seems to be working correctly.

@Abdenasser
Copy link
Owner

@Abdenasser Please take a look at it again, I have tested all functionality and everything seems to be working correctly.

great job @Pinguin2001

@Abdenasser Abdenasser self-requested a review November 6, 2024 18:56
Copy link
Owner

@Abdenasser Abdenasser left a comment

Choose a reason for hiding this comment

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

LGTM

@Abdenasser Abdenasser merged commit 6f719a7 into Abdenasser:main Nov 6, 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.

3 participants