[v3 alpha] Fix deadlock of linux dialog for multiple selections #3925
Conversation
WalkthroughThe pull request updates the changelog and modifies the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
michael-freling
force-pushed
the
fix-v3
branch
from
a331204
to
74b33da
Compare
michael-freling
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
mkdocs-website/docs/en/changelog.md (1)
48-48: Consider enhancing the changelog entry with more technical details.While the entry correctly documents the fix, it would be more helpful for future reference to include details about the root cause. Consider expanding it to:
- - Fixed deadlock of linux dialog for multiple selections by @michael-freling in [#3925](https://github.com/wailsapp/wails/pull/3925) + - Fixed deadlock in Linux dialog for multiple selections caused by unclosed channel variable by @michael-freling in [#3925](https://github.com/wailsapp/wails/pull/3925)This addition provides valuable context about the specific issue that was fixed.
v3/pkg/application/linux_cgo.go (2)
Line range hint
1481-1499: Avoid global variables for debounce logic to prevent concurrency issuesThe variables
debounceTimerandisDebouncingare declared at the package level, which can lead to race conditions when multiple windows are involved. It's recommended to move these variables into thelinuxWebviewWindowstruct to ensure thread safety.Apply the following changes to encapsulate the debounce logic within each window instance:
- var debounceTimer *time.Timer - var isDebouncing bool = false + // Add these fields to the linuxWebviewWindow struct + type linuxWebviewWindow struct { + // ... existing fields ... + debounceTimer *time.Timer + isDebouncing bool + // ... existing fields ... + }Update the
onKeyPressEventfunction to use the window-specific variables:func onKeyPressEvent(_ *C.GtkWidget, event *C.GdkEventKey, userData C.uintptr_t) C.gboolean { + windowID := uint(C.uint(userData)) + window := globalApplication.getWindowForID(windowID) + if window == nil { + return C.gboolean(0) + } + lw, ok := window.(*WebviewWindow).impl.(*linuxWebviewWindow) + if !ok { + return C.gboolean(0) + } - if isDebouncing { - debounceTimer.Reset(50 * time.Millisecond) + if lw.isDebouncing { + lw.debounceTimer.Reset(50 * time.Millisecond) return C.gboolean(0) } - isDebouncing = true - debounceTimer = time.AfterFunc(50*time.Millisecond, func() { - isDebouncing = false + lw.isDebouncing = true + lw.debounceTimer = time.AfterFunc(50*time.Millisecond, func() { + lw.isDebouncing = false }) - windowID := uint(C.uint(userData)) if accelerator, ok := getKeyboardState(event); ok { windowKeyEvents <- &windowKeyEvent{ windowId: windowID, acceleratorString: accelerator, } } return C.gboolean(0) }
Line range hint
1358-1365: Correct the zoom factor inzoomOutmethodIn the
zoomOutmethod, multiplying the zoom level by a negative factor-1.10is incorrect and can cause unexpected behavior. To decrease the zoom level properly, you should divide the current zoom level by a positive zoom factor.Apply the following changes to fix the zoom in and zoom out functionality:
func (w *linuxWebviewWindow) zoomIn() { - ZoomInFactor := 1.10 + zoomFactor := 1.10 w.setZoom(w.getZoom() * zoomFactor) } func (w *linuxWebviewWindow) zoomOut() { - ZoomInFactor := -1.10 - w.setZoom(w.getZoom() * ZoomInFactor) + zoomFactor := 1.10 + w.setZoom(w.getZoom() / zoomFactor) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/pkg/application/linux_cgo.go(1 hunks)
🔇 Additional comments (1)
mkdocs-website/docs/en/changelog.md (1)
Line range hint 1-24: LGTM! Changelog follows best practices.
The changelog is well-structured and follows the Keep a Changelog format. It properly categorizes changes, includes clear attributions, and maintains consistent formatting throughout.
🧰 Tools
🪛 LanguageTool
[duplication] ~47-~47: Possible typo: you repeated a word
Context: ...ny](https://github.com/leaanthony) ### Fixed - Fixed deadlock of linux dialog for multiple s...
(ENGLISH_WORD_REPEAT_RULE)
michael-freling
force-pushed
the
fix-v3
branch
from
93a90c2
to
74ac939
Compare
|
Thanks for taking the time to open this. Looks like there's a merge conflict. Once that's fixed, and @atterpac is happy with it, we can merge it 🚀 |
Head branch was pushed to by a user without write access
michael-freling
force-pushed
the
fix-v3
branch
from
74ac939
to
7b4befb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
v3/pkg/application/linux_cgo.go (3)
Line range hint
988-992: Simplify theisVisiblefunction by returning the boolean expression directlyThe
isVisiblefunction can be simplified to return the result of the comparison directly, improving code readability.Apply this diff to simplify the function:
-func (w *linuxWebviewWindow) isVisible() bool { - if C.gtk_widget_is_visible(w.gtkWidget()) == 1 { - return true - } - return false +func (w *linuxWebviewWindow) isVisible() bool { + return C.gtk_widget_is_visible(w.gtkWidget()) == 1 }
Line range hint
1664-1681: Avoid unnecessary goroutine inrunChooserDialogto prevent potential deadlocksThe use of
go func()insideInvokeAsyncmay lead to concurrency issues or potential deadlocks. SinceInvokeAsyncalready handles asynchronous execution, starting a new goroutine is unnecessary.Apply this diff to remove the redundant goroutine:
InvokeAsync(func() { response := C.gtk_dialog_run((*C.GtkDialog)(fc)) - go func() { if response == C.GTK_RESPONSE_ACCEPT { filenames := C.gtk_file_chooser_get_filenames((*C.GtkFileChooser)(fc)) iter := filenames count := 0 for { selections <- buildStringAndFree(C.gpointer(iter.data)) iter = iter.next if iter == nil || count == 1024 { break } count++ } } close(selections) - }() })
Line range hint
1819-1825: Remove commented code to improve code cleanlinessThe methods
undo,redo, anddeletecontain commented-out code. It's good practice to remove such code to maintain code clarity and cleanliness.Apply this diff to clean up the methods:
func (w *linuxWebviewWindow) undo() { - //C.webkit_web_view_execute_editing_command(w.webview, C.WEBKIT_EDITING_COMMAND_UNDO) } func (w *linuxWebviewWindow) redo() { -} +} func (w *linuxWebviewWindow) delete() { -} +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/pkg/application/linux_cgo.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mkdocs-website/docs/en/changelog.md
|
@leaanthony Thank you. I resolved the conflict. |
Description
The channel variable for the selections of a dialog isn't closed on Linux.
As a result, the next statement waits a value from the channel forever.
wails/v3/pkg/application/dialogs.go
Lines 298 to 301 in 8444ccf
I have found no issue reported about this bug.
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.wails3 doctor output
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Bug Fixes
Changes