-
Notifications
You must be signed in to change notification settings - Fork 451
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
Fix Flet 0.23 crash on Ubuntu 22.04 #3495
Conversation
Reviewer's Guide by SourceryThis pull request addresses issue #3490 by fixing a crash in Flet 0.23 on Ubuntu 22.04. The changes involve modifications to the File-Level Changes
Tips
|
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.
Hey @FeodorFitsner - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -48,7 +48,7 @@ static void my_application_activate(GApplication* application) { | |||
} | |||
|
|||
gtk_window_set_default_size(window, 1280, 720); | |||
gtk_widget_realize(GTK_WIDGET(window)); | |||
gtk_widget_show(GTK_WIDGET(window)); |
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.
suggestion: Consider using gtk_widget_show_all for better visibility.
Using gtk_widget_show_all(GTK_WIDGET(window)) instead of gtk_widget_show(GTK_WIDGET(window)) will ensure that all child widgets are also shown, which might be necessary depending on the window's content.
gtk_widget_show(GTK_WIDGET(window)); | |
gtk_widget_show_all(GTK_WIDGET(window)); |
@@ -81,6 +81,24 @@ static gboolean my_application_local_command_line(GApplication* application, gch | |||
return TRUE; | |||
} | |||
|
|||
// Implements GApplication::startup. | |||
static void my_application_startup(GApplication* application) { | |||
//MyApplication* self = MY_APPLICATION(object); |
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.
suggestion: Remove commented-out code if not needed.
If the commented-out code is not necessary, it is better to remove it to keep the codebase clean. If it is needed for future reference, consider adding a comment explaining why it is commented out.
//MyApplication* self = MY_APPLICATION(object); | |
// If needed for future reference, uncomment and use the following line: | |
// MyApplication* self = MY_APPLICATION(object); |
@@ -122,6 +123,12 @@ foreach(bundled_library ${PLUGIN_BUNDLED_LIBRARIES}) | |||
COMPONENT Runtime) | |||
endforeach(bundled_library) | |||
|
|||
# Copy the native assets provided by the build.dart from all packages. | |||
set(NATIVE_ASSETS_DIR "${PROJECT_BUILD_DIR}native_assets/linux/") |
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.
issue (documentation): Possible missing slash in directory path.
Should there be a slash after ${PROJECT_BUILD_DIR}
in the NATIVE_ASSETS_DIR
path?
Fix #3490
Summary by Sourcery
This pull request addresses a crash issue in Flet 0.23 on Ubuntu 22.04 by modifying the GTK window initialization. It also enhances the application lifecycle management by adding startup and shutdown methods, and updates the build configuration to handle native assets.