feat: theme-patch#1
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom cosmic-dark theme and UI layout for Gozik, replacing the native file chooser with a custom, themeable file dialog, adding command-line file loading, and implementing background duration probing for the queue. It also adds karaoke-style synced lyrics highlighting. The review identified several critical and medium-severity issues: a compilation error in window.go due to an incorrect number of return values from CreateTag, a potential out-of-bounds panic in humanSize for files larger than 1 PB, a fragile file extension extraction method in player.go, and the use of unsupported CSS animations/keyframes in GTK3.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if mw.lyricTagNow == nil { | ||
| mw.lyricTagNow = b.CreateTag("now", map[string]interface{}{"foreground": "#6FE0EE"}) | ||
| } | ||
| if mw.lyricTagSung == nil { | ||
| mw.lyricTagSung = b.CreateTag("sung", map[string]interface{}{"foreground": "#E8C879"}) | ||
| } |
There was a problem hiding this comment.
In gotk3, TextBuffer.CreateTag returns two values: (*TextTag, error). Assigning it to a single variable (mw.lyricTagNow / mw.lyricTagSung) will cause a compilation error: assignment mismatch: 1 variable but b.CreateTag returns 2 values. You must receive or ignore the error value to allow the code to compile.
| if mw.lyricTagNow == nil { | |
| mw.lyricTagNow = b.CreateTag("now", map[string]interface{}{"foreground": "#6FE0EE"}) | |
| } | |
| if mw.lyricTagSung == nil { | |
| mw.lyricTagSung = b.CreateTag("sung", map[string]interface{}{"foreground": "#E8C879"}) | |
| } | |
| if mw.lyricTagNow == nil { | |
| mw.lyricTagNow, _ = b.CreateTag("now", map[string]interface{}{"foreground": "#6FE0EE"}) | |
| } | |
| if mw.lyricTagSung == nil { | |
| mw.lyricTagSung, _ = b.CreateTag("sung", map[string]interface{}{"foreground": "#E8C879"}) | |
| } |
| if err != nil { | ||
| return 0, err | ||
| } | ||
| ext := strings.ToLower(filename[strings.LastIndex(filename, ".")+1:]) |
There was a problem hiding this comment.
Using strings.LastIndex(filename, ".") to extract the file extension is prone to bugs if the directory path contains a dot but the file itself does not have an extension (e.g., /path.to/audiofile). It is much safer and more robust to use filepath.Ext(filename) from the "path/filepath" package (make sure to import it).
| ext := strings.ToLower(filename[strings.LastIndex(filename, ".")+1:]) | |
| ext := strings.ToLower(strings.TrimPrefix(filepath.Ext(filename), ".")) |
| func humanSize(n int64) string { | ||
| const u = 1024 | ||
| if n < u { | ||
| return fmt.Sprintf("%d B", n) | ||
| } | ||
| div, exp := int64(u), 0 | ||
| for x := n / u; x >= u; x /= u { | ||
| div *= u | ||
| exp++ | ||
| } | ||
| return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGT"[exp]) | ||
| } |
There was a problem hiding this comment.
The humanSize function can panic with an "index out of range" error if the file size n is extremely large (1 PB or more). For a size of 1 PB or larger, exp will exceed 3, causing "KMGT"[exp] to panic because the string only has 4 characters (indices 0 to 3). Expanding the units string to "KMGTPE" (to include Peta and Exa) will safely cover the entire range of a 64-bit signed integer (int64 maxes out at ~8 Exabytes).
| func humanSize(n int64) string { | |
| const u = 1024 | |
| if n < u { | |
| return fmt.Sprintf("%d B", n) | |
| } | |
| div, exp := int64(u), 0 | |
| for x := n / u; x >= u; x /= u { | |
| div *= u | |
| exp++ | |
| } | |
| return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGT"[exp]) | |
| } | |
| func humanSize(n int64) string { | |
| const u = 1024 | |
| if n < u { | |
| return fmt.Sprintf("%d B", n) | |
| } | |
| div, exp := int64(u), 0 | |
| for x := n / u; x >= u; x /= u { | |
| div *= u | |
| exp++ | |
| } | |
| return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGTPE"[exp]) | |
| } |
| list row.playing #row-lead { | ||
| color: @gold; | ||
| animation: gozik-pulse 2.4s ease-in-out infinite; | ||
| } | ||
| @keyframes gozik-pulse { | ||
| 0% { opacity: 0.40; } | ||
| 50% { opacity: 1; } | ||
| 100% { opacity: 0.40; } | ||
| } |
There was a problem hiding this comment.
GTK3's CSS engine (GtkCssProvider) does not support CSS @keyframes or the animation property. Defining animation: gozik-pulse ... and @keyframes gozik-pulse will result in silent parsing failures or warnings, and the pulse effect will not render. Consider using GTK transitions or handling the pulsing effect programmatically if needed.
|
일단 제일 먼저, 제가 로컬에서 작업하던 상황(CD 관련, ffmpeg)를 @rabbitson87님의 작업 상황에 맞게 머지하는 커밋을 작성 후, ui 깨짐 여부를 확인하고 머지하겠습니다. UI/UX 관점에서 전문적이고 상세한 구현을 해 주심에 감사드립니다. |
|
그리고, Repo 소개란에 여전히 구 명칭인 Dionysus Music Player로 기록되어 있는데, 쓰기 권한이 있으신 만큼이나 Gozik으로 이름을 바꿔 주셨으면 좋겠습니다. |
네 변경했어요 확인부탁드립니다. |
|
LGTM bb |
2 participants
No description provided.