Add basic gui tray app#22
Conversation
|
Warning Rate limit exceeded@Agent-Hellboy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughThe changes introduce a new function for setting clipboard content using X11, expand the Debian packaging script to include a GUI tray app, add a systemd service template for the tray app, and update documentation with instructions for using the man page and starting the tray app. New dependencies and a man page are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Systemd
participant TrayApp
participant X11Server
User->>Systemd: Enable/start hcp-tray@user.service
Systemd->>TrayApp: Launch hcp-tray process
TrayApp->>X11Server: Connect and listen for clipboard changes
TrayApp->>X11Server: Set clipboard content (set_clipboard)
X11Server->>TrayApp: Send SelectionRequest event
TrayApp->>X11Server: Respond with clipboard data
Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
README.md (2)
138-138: Add language specification to code block.The fenced code block should specify a language for better syntax highlighting.
-``` +```bash man hcp--- `148-151`: **Add language specification to code block.** The fenced code block should specify a language for better syntax highlighting. ```diff -``` +```bash sudo systemctl enable hcp-tray@$(whoami).service sudo systemctl start hcp-tray@$(whoami).service</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f4a0c2accd5cbbb5857d92f6aed56fd79a97a4b1 and 8e6cbea1388c65f9f4bd448531ca8c9374b3f00a. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `README.md` (1 hunks) * `build_deb.sh` (3 hunks) * `include/clipboard_mgmt.hpp` (1 hunks) * `src/clipboard_mgmt.cpp` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>README.md</summary> [grammar] ~134-~134: Use proper spacing conventions. Context: ... See [LICENSE](LICENSE). ## Manual Page After installing the package, you can vi... (QB_NEW_EN_OTHER_ERROR_IDS_000007) --- [grammar] ~136-~136: Use proper spacing conventions. Context: ...w the manual page for usage and options: ``` man hcp ``` This provides detailed information about... (QB_NEW_EN_OTHER_ERROR_IDS_000007) --- [grammar] ~142-~142: Use proper spacing conventions. Context: ...t available commands and usage examples. ## Starting the GUI Tray App After install... (QB_NEW_EN_OTHER_ERROR_IDS_000007) --- [grammar] ~144-~144: Use proper spacing conventions. Context: ... examples. ## Starting the GUI Tray App After installing, you can enable and sta... (QB_NEW_EN_OTHER_ERROR_IDS_000007) --- [grammar] ~146-~146: Use proper spacing conventions. Context: ...em tray and provides clipboard history): ``` sudo systemctl enable hcp-tray@$(whoami).service sudo systemctl start hcp-tray@$(whoami).service ``` This will launch the tray app in the bac... (QB_NEW_EN_OTHER_ERROR_IDS_000007) --- [grammar] ~153-~153: Use proper spacing conventions. Context: ...in the background for your user session. (QB_NEW_EN_OTHER_ERROR_IDS_000007) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>README.md</summary> 138-138: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 148-148: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>include/clipboard_mgmt.hpp (1)</summary> `7-7`: **Function declaration looks good.** The new `set_clipboard` function declaration is consistent with the existing code style and uses appropriate parameter types for X11 clipboard operations. </details> <details> <summary>README.md (1)</summary> `134-153`: **Documentation additions look good.** The new sections provide clear instructions for users to access the man page and start the GUI tray app. The content is well-structured and helpful. </details> <details> <summary>build_deb.sh (2)</summary> `27-27`: **Dependencies added correctly.** The new GTK and AppIndicator dependencies are appropriate for the GUI tray app functionality. --- `63-65`: **Post-install instructions updated appropriately.** The addition of tray app instructions in the post-install script provides helpful guidance for users. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| void set_clipboard(Display *display, const std::string &text) { | ||
| Atom clipboard = XInternAtom(display, "CLIPBOARD", False); | ||
| Atom utf8 = XInternAtom(display, "UTF8_STRING", False); | ||
| Atom targets[] = {utf8, XA_STRING}; | ||
|
|
||
| Window window = XCreateSimpleWindow(display, DefaultRootWindow(display), 0, 0, | ||
| 1, 1, 0, 0, 0); | ||
| XSetSelectionOwner(display, clipboard, window, CurrentTime); | ||
|
|
||
| XEvent event; | ||
| bool running = true; | ||
| while (running) { | ||
| XNextEvent(display, &event); | ||
|
|
||
| if (event.type == SelectionRequest) { | ||
| XSelectionRequestEvent *req = &event.xselectionrequest; | ||
| XEvent respond; | ||
| memset(&respond, 0, sizeof(respond)); | ||
| respond.xselection.type = SelectionNotify; | ||
| respond.xselection.display = req->display; | ||
| respond.xselection.requestor = req->requestor; | ||
| respond.xselection.selection = req->selection; | ||
| respond.xselection.target = req->target; | ||
| respond.xselection.time = req->time; | ||
| respond.xselection.property = None; | ||
|
|
||
| if (req->target == utf8 || req->target == XA_STRING) { | ||
| XChangeProperty(display, req->requestor, req->property, req->target, 8, | ||
| PropModeReplace, (const unsigned char *)text.c_str(), | ||
| text.length()); | ||
| respond.xselection.property = req->property; | ||
| } | ||
| XSendEvent(display, req->requestor, 0, 0, &respond); | ||
| XFlush(display); | ||
| running = false; // Only serve one request | ||
| } | ||
| } | ||
| XDestroyWindow(display, window); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Critical issue: Clipboard ownership model is flawed.
The current implementation has several serious problems:
-
Single-use limitation: The function exits after serving one
SelectionRequest(line 235), meaning clipboard data will only be available for one paste operation. Subsequent paste attempts will fail. -
Infinite blocking: The event loop has no timeout mechanism - if no
SelectionRequestarrives, the function will block indefinitely. -
Missing error handling: No verification that
XSetSelectionOwnersucceeded or that window creation didn't fail.
For proper clipboard management, consider one of these approaches:
Option 1: Persistent ownership (recommended for GUI tray app)
-void set_clipboard(Display *display, const std::string &text) {
+void set_clipboard(Display *display, const std::string &text, bool persistent = true) {
// ... existing setup code ...
XEvent event;
- bool running = true;
+ bool running = persistent;
while (running) {
XNextEvent(display, &event);
if (event.type == SelectionRequest) {
// ... existing handling code ...
- running = false; // Only serve one request
+ if (!persistent) running = false; // Only serve one request for CLI usage
}
}
XDestroyWindow(display, window);
}Option 2: Add timeout for non-persistent usage
+ fd_set readfds;
+ struct timeval timeout;
+ int x11_fd = ConnectionNumber(display);
+
while (running) {
+ FD_ZERO(&readfds);
+ FD_SET(x11_fd, &readfds);
+ timeout.tv_sec = 30; // 30 second timeout
+ timeout.tv_usec = 0;
+
+ if (select(x11_fd + 1, &readfds, NULL, NULL, &timeout) <= 0) {
+ break; // Timeout or error
+ }
+
+ if (XPending(display)) {
XNextEvent(display, &event);
// ... rest of event handling
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/clipboard_mgmt.cpp around lines 201 to 239, the clipboard ownership is
handled incorrectly by serving only one SelectionRequest and then exiting,
causing clipboard data to be unavailable for subsequent paste operations. To fix
this, modify the event loop to persistently handle multiple SelectionRequest
events instead of stopping after one, ensuring continuous clipboard ownership.
Additionally, add error handling to verify that XCreateSimpleWindow and
XSetSelectionOwner succeed, and implement a timeout mechanism in the event loop
to prevent indefinite blocking if no requests arrive. This will make clipboard
management robust and reliable.
| cat > $PKGDIR/etc/systemd/system/hcp-tray@.service <<EOF | ||
| [Unit] | ||
| Description=HCP Clipboard Tray App | ||
| After=graphical-session.target | ||
|
|
||
| [Service] | ||
| Type=simple | ||
| User=%i | ||
| Environment=DISPLAY=:0 | ||
| Environment=XAUTHORITY=/home/%i/.Xauthority | ||
| ExecStart=/usr/bin/hcp-tray | ||
| Restart=on-failure | ||
|
|
||
| [Install] | ||
| WantedBy=default.target | ||
| EOF |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix systemd service configuration inconsistency.
The service uses After=graphical-session.target but WantedBy=default.target. For a GUI tray app, consider using graphical-session.target for both or verify the intended behavior.
[Unit]
Description=HCP Clipboard Tray App
-After=graphical-session.target
+After=graphical-session.target
[Service]
Type=simple
User=%i
Environment=DISPLAY=:0
Environment=XAUTHORITY=/home/%i/.Xauthority
ExecStart=/usr/bin/hcp-tray
Restart=on-failure
[Install]
-WantedBy=default.target
+WantedBy=graphical-session.target📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cat > $PKGDIR/etc/systemd/system/hcp-tray@.service <<EOF | |
| [Unit] | |
| Description=HCP Clipboard Tray App | |
| After=graphical-session.target | |
| [Service] | |
| Type=simple | |
| User=%i | |
| Environment=DISPLAY=:0 | |
| Environment=XAUTHORITY=/home/%i/.Xauthority | |
| ExecStart=/usr/bin/hcp-tray | |
| Restart=on-failure | |
| [Install] | |
| WantedBy=default.target | |
| EOF | |
| cat > $PKGDIR/etc/systemd/system/hcp-tray@.service <<EOF | |
| [Unit] | |
| Description=HCP Clipboard Tray App | |
| After=graphical-session.target | |
| [Service] | |
| Type=simple | |
| User=%i | |
| Environment=DISPLAY=:0 | |
| Environment=XAUTHORITY=/home/%i/.Xauthority | |
| ExecStart=/usr/bin/hcp-tray | |
| Restart=on-failure | |
| [Install] | |
| WantedBy=graphical-session.target | |
| EOF |
🤖 Prompt for AI Agents
In build_deb.sh around lines 78 to 93, the systemd service file inconsistently
uses After=graphical-session.target but WantedBy=default.target. To fix this,
update the WantedBy directive to graphical-session.target to match the After
directive, ensuring consistent and correct service startup ordering for the GUI
tray app.
| # Build the GUI tray app | ||
| cd gui && make && cd .. | ||
| cp gui/hcp-tray $PKGDIR/usr/bin/hcp-tray |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for GUI build process.
The build process assumes the gui directory exists and has a working Makefile without verification.
-# Build the GUI tray app
-cd gui && make && cd ..
-cp gui/hcp-tray $PKGDIR/usr/bin/hcp-tray
+# Build the GUI tray app
+if [ ! -d "gui" ]; then
+ echo "Error: gui directory not found"
+ exit 1
+fi
+cd gui && make && cd ..
+if [ ! -f "gui/hcp-tray" ]; then
+ echo "Error: hcp-tray binary not built"
+ exit 1
+fi
+cp gui/hcp-tray $PKGDIR/usr/bin/hcp-tray📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Build the GUI tray app | |
| cd gui && make && cd .. | |
| cp gui/hcp-tray $PKGDIR/usr/bin/hcp-tray | |
| # Build the GUI tray app | |
| if [ ! -d "gui" ]; then | |
| echo "Error: gui directory not found" | |
| exit 1 | |
| fi | |
| cd gui && make && cd .. | |
| if [ ! -f "gui/hcp-tray" ]; then | |
| echo "Error: hcp-tray binary not built" | |
| exit 1 | |
| fi | |
| cp gui/hcp-tray $PKGDIR/usr/bin/hcp-tray |
🤖 Prompt for AI Agents
In build_deb.sh around lines 73 to 75, the script assumes the gui directory
exists and the make command succeeds without checking. Add error handling by
first verifying the gui directory exists before running make, and check the exit
status of the make command. If the directory is missing or make fails, output an
error message and exit the script to prevent further steps from running on a
failed build.
| cat > $PKGDIR/usr/share/man/man1/hcp.1 <<'EOF' | ||
| .TH hcp 1 "$(date +%Y-%m-%d)" "$VERSION" "hcp manual" | ||
| .SH NAME | ||
| hcp \- Historical Clipboard Manager for X11 | ||
| .SH SYNOPSIS | ||
| .B hcp | ||
| [service start | list | <index> | pop | --help | -h] | ||
| .SH DESCRIPTION | ||
| .B hcp | ||
| is a lightweight clipboard manager for X11 systems. It captures clipboard entries, maintains a history, and allows you to list, print, or remove entries. Designed for reliability and minimalism, it works directly with the X11 clipboard and is suitable for use as a background service or on-demand. | ||
| .SH COMMANDS | ||
| .TP | ||
| .B service start | ||
| Start clipboard monitoring service in the background. | ||
| .TP | ||
| .B list | ||
| List clipboard history. | ||
| .TP | ||
| .B <index> | ||
| Print clipboard entry at <index>. | ||
| .TP | ||
| .B pop | ||
| Remove most recent clipboard entry. | ||
| .TP | ||
| .B --help, -h | ||
| Show this help message. | ||
| .SH AUTHOR | ||
| Written by the hcp project contributors. | ||
| .SH SEE ALSO | ||
| clipboard(1) | ||
| EOF | ||
| gzip -f $PKGDIR/usr/share/man/man1/hcp.1 |
There was a problem hiding this comment.
Fix variable expansion in man page creation.
The man page uses $(date +%Y-%m-%d) and $VERSION in a single-quoted heredoc, which prevents variable expansion.
-cat > $PKGDIR/usr/share/man/man1/hcp.1 <<'EOF'
-.TH hcp 1 "$(date +%Y-%m-%d)" "$VERSION" "hcp manual"
+cat > $PKGDIR/usr/share/man/man1/hcp.1 <<EOF
+.TH hcp 1 "$(date +%Y-%m-%d)" "$VERSION" "hcp manual"Or use a different approach:
+# Create the man page with proper variable expansion
+MANPAGE_DATE=$(date +%Y-%m-%d)
+cat > $PKGDIR/usr/share/man/man1/hcp.1 <<EOF
+.TH hcp 1 "$MANPAGE_DATE" "$VERSION" "hcp manual"🤖 Prompt for AI Agents
In build_deb.sh around lines 97 to 128, the man page is created using a
single-quoted heredoc which prevents the variables $(date +%Y-%m-%d) and
$VERSION from expanding. To fix this, change the heredoc delimiter from single
quotes to unquoted or double quotes so that shell variables are expanded
properly during the cat command execution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
25-28: “List workspace files” step produces noisy logs—consider dropping or gating behind debug flag
ls -lRwill dump the whole repository tree into the CI log on every PR.
This is useful once when troubleshooting, but quickly becomes spam and can expose directory layout to forks.- - name: List workspace files - run: ls -lR +# - name: List workspace files # uncomment for ad-hoc debugging +# if: ${{ github.event_name == 'workflow_dispatch' && inputs.debug }} +# run: ls -lRKeeping the command commented or conditioned on an explicit
debugflag keeps routine runs lean while preserving the option to inspect files when needed.
30-33: Package list can be trimmed & made non-interactive
libx11-devalready depends onlibx11-6; installing both is redundant.- Adding
DEBIAN_FRONTEND=noninteractiveavoids any edge-case prompts.--no-install-recommendsspeeds up CI by skipping extras you don’t ship.- sudo apt-get install -y libx11-dev libx11-6 dpkg xvfb xclip libgtk-3-dev libappindicator3-dev + sudo DEBIAN_FRONTEND=noninteractive \ + apt-get install -y --no-install-recommends \ + dpkg xvfb xclip libx11-dev libgtk-3-dev libappindicator3-devThis shaves a few seconds off every CI run and reduces network traffic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
30-33: Confirmlibappindicator3-devavailability onubuntu-latestThe sandbox environment didn’t include
apt-cache, so we couldn’t verify package availability automatically. Please manually confirm thatlibappindicator3-devis present in the universe repo on your target runner (e.g. by runningapt-cache policy libappindicator3-devon bothubuntu-22.04and any futureubuntu-24.04images). To guard against breakage:
- Pin the workflow to a known image:
runs-on: ubuntu-22.04- Or add a fail-fast check before installing:
- run: | if ! apt-cache policy libappindicator3-dev | grep -q 'Candidate:'; then echo 'libappindicator3-dev not found on runner' >&2 exit 1 fi sudo apt-get update sudo apt-get install -y libx11-dev libx11-6 dpkg xvfb xclip libgtk-3-dev libappindicator3-dev
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
gui/trey_app.cpp (2)
32-32: Make text truncation configurable and add ellipsis.The hardcoded 60-character limit may not be suitable for all use cases and should provide visual indication of truncation.
- GtkWidget *label = gtk_label_new(entry.substr(0, 60).c_str()); + constexpr size_t MAX_LABEL_LENGTH = 60; + std::string label_text = entry.length() > MAX_LABEL_LENGTH ? + entry.substr(0, MAX_LABEL_LENGTH - 3) + "..." : entry; + GtkWidget *label = gtk_label_new(label_text.c_str());
19-21: Add proper window management and user experience improvements.The window lacks proper parent-child relationship and should be centered on screen.
GtkWidget *window = gtk_window_new(GTK_WINDOW_TOPLEVEL); gtk_window_set_title(GTK_WINDOW(window), "Clipboard History"); gtk_window_set_default_size(GTK_WINDOW(window), 400, 400); + gtk_window_set_position(GTK_WINDOW(window), GTK_WIN_POS_CENTER); + gtk_window_set_resizable(GTK_WINDOW(window), TRUE); + g_signal_connect(window, "destroy", G_CALLBACK(gtk_widget_destroy), NULL);gui/Makefile (1)
10-11: Add object file tracking and parallel build support.The current build system recompiles all sources on every change, which is inefficient.
+OBJS = $(SRC:.cpp=.o) + -$(TARGET): $(SRC) - $(CXX) $(CXXFLAGS) -o $@ $^ $(LDFLAGS) +$(TARGET): $(OBJS) + $(CXX) -o $@ $^ $(LDFLAGS) + +%.o: %.cpp + $(CXX) $(CXXFLAGS) -c $< -o $@And update the clean target:
clean: - rm -f $(TARGET) + rm -f $(TARGET) $(OBJS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gui/Makefile(1 hunks)gui/trey_app.cpp(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
gui/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 8-8: Target "all" should be declared PHONY.
(phonydeclared)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (2)
gui/trey_app.cpp (2)
46-50: Verify AppIndicator icon availability and implement a fallbackHardcoding
"system-run-symbolic"risks an invisible tray icon if that name isn’t provided by the user’s icon theme. At runtime, check for the primary icon and fall back to a known default (e.g."application-default-icon").• File: gui/trey_app.cpp Lines 46–50
• Usegtk_icon_theme_get_default()+gtk_icon_theme_has_icon()(or equivalent) to detect presenceSuggested diff:
- const char *icon_name = "system-run-symbolic"; + const char *icon_name = "system-run-symbolic"; + GtkIconTheme *theme = gtk_icon_theme_get_default(); + if (!gtk_icon_theme_has_icon(theme, icon_name)) { + icon_name = "application-default-icon"; + } AppIndicator *indicator = app_indicator_new( "clipboard-manager", - "system-run-symbolic", + icon_name, APP_INDICATOR_CATEGORY_APPLICATION_STATUS );
43-44: Add command-line argument validation.The GTK initialization should validate command-line arguments properly.
int main(int argc, char **argv) { + if (argc < 1 || argv == nullptr) { + g_error("Invalid command line arguments"); + return 1; + } gtk_init(&argc, &argv);Likely an incorrect or invalid review comment.
| Display *display = XOpenDisplay(nullptr); | ||
| if (display) { | ||
| set_clipboard(display, std::string(text)); | ||
| XCloseDisplay(display); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for clipboard operations.
The function should handle potential failures in clipboard operations and provide user feedback.
static void on_copy_clicked(GtkButton *button, gpointer user_data) {
const char *text = (const char *)user_data;
Display *display = XOpenDisplay(nullptr);
if (display) {
- set_clipboard(display, std::string(text));
+ try {
+ set_clipboard(display, std::string(text));
+ } catch (const std::exception &e) {
+ g_warning("Failed to set clipboard: %s", e.what());
+ }
XCloseDisplay(display);
+ } else {
+ g_warning("Failed to open X11 display");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Display *display = XOpenDisplay(nullptr); | |
| if (display) { | |
| set_clipboard(display, std::string(text)); | |
| XCloseDisplay(display); | |
| } | |
| static void on_copy_clicked(GtkButton *button, gpointer user_data) { | |
| const char *text = (const char *)user_data; | |
| Display *display = XOpenDisplay(nullptr); | |
| if (display) { | |
| try { | |
| set_clipboard(display, std::string(text)); | |
| } catch (const std::exception &e) { | |
| g_warning("Failed to set clipboard: %s", e.what()); | |
| } | |
| XCloseDisplay(display); | |
| } else { | |
| g_warning("Failed to open X11 display"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In gui/trey_app.cpp around lines 11 to 15, the clipboard operations lack error
handling. Modify the code to check the success of set_clipboard and handle any
failures by providing appropriate user feedback, such as logging an error
message or notifying the user. Ensure that XOpenDisplay failure is also handled
gracefully by informing the user if the display cannot be opened.
| GtkWidget *vbox = gtk_box_new(GTK_ORIENTATION_VERTICAL, 5); | ||
| gtk_container_add(GTK_CONTAINER(scrolled), vbox); | ||
|
|
||
| std::vector<std::string> history = load_clipboard_blocks(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database operations.
The load_clipboard_blocks() function call should handle potential database errors gracefully.
- std::vector<std::string> history = load_clipboard_blocks();
+ std::vector<std::string> history;
+ try {
+ history = load_clipboard_blocks();
+ } catch (const std::exception &e) {
+ g_warning("Failed to load clipboard history: %s", e.what());
+ // Show error message to user
+ GtkWidget *error_label = gtk_label_new("Error loading clipboard history");
+ gtk_container_add(GTK_CONTAINER(vbox), error_label);
+ gtk_widget_show_all(window);
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::vector<std::string> history = load_clipboard_blocks(); | |
| // Before: std::vector<std::string> history = load_clipboard_blocks(); | |
| std::vector<std::string> history; | |
| try { | |
| history = load_clipboard_blocks(); | |
| } catch (const std::exception &e) { | |
| g_warning("Failed to load clipboard history: %s", e.what()); | |
| // Show error message to user | |
| GtkWidget *error_label = gtk_label_new("Error loading clipboard history"); | |
| gtk_container_add(GTK_CONTAINER(vbox), error_label); | |
| gtk_widget_show_all(window); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In gui/trey_app.cpp at line 29, the call to load_clipboard_blocks() lacks error
handling for potential database failures. Modify the code to catch exceptions or
check error return values from load_clipboard_blocks(), and handle these errors
gracefully by logging the issue or providing fallback behavior to prevent
crashes or undefined states.
| static void on_copy_clicked(GtkButton *button, gpointer user_data) { | ||
| const char *text = (const char *)user_data; | ||
| Display *display = XOpenDisplay(nullptr); | ||
| if (display) { | ||
| set_clipboard(display, std::string(text)); | ||
| XCloseDisplay(display); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical memory safety issue with dangling pointer.
The entry.c_str() pointer passed as user_data will become invalid after the entry object goes out of scope in the show_history_window function. This creates a dangling pointer that can cause crashes or undefined behavior when the copy button is clicked.
Apply this diff to fix the memory safety issue by copying the string data:
- g_signal_connect(button, "clicked", G_CALLBACK(on_copy_clicked), (gpointer)entry.c_str());
+ char *entry_copy = g_strdup(entry.c_str());
+ g_signal_connect(button, "clicked", G_CALLBACK(on_copy_clicked), entry_copy);
+ g_object_set_data_full(G_OBJECT(button), "entry_data", entry_copy, g_free);And update the on_copy_clicked function to handle the copied data:
static void on_copy_clicked(GtkButton *button, gpointer user_data) {
- const char *text = (const char *)user_data;
+ const char *text = (const char *)g_object_get_data(G_OBJECT(button), "entry_data");
Display *display = XOpenDisplay(nullptr);
if (display) {
set_clipboard(display, std::string(text));
XCloseDisplay(display);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void on_copy_clicked(GtkButton *button, gpointer user_data) { | |
| const char *text = (const char *)user_data; | |
| Display *display = XOpenDisplay(nullptr); | |
| if (display) { | |
| set_clipboard(display, std::string(text)); | |
| XCloseDisplay(display); | |
| } | |
| } | |
| static void on_copy_clicked(GtkButton *button, gpointer user_data) { | |
| - const char *text = (const char *)user_data; | |
| + const char *text = (const char *)g_object_get_data(G_OBJECT(button), "entry_data"); | |
| Display *display = XOpenDisplay(nullptr); | |
| if (display) { | |
| set_clipboard(display, std::string(text)); | |
| XCloseDisplay(display); | |
| } | |
| } |
🤖 Prompt for AI Agents
In gui/trey_app.cpp around lines 9 to 16, the user_data pointer passed to
on_copy_clicked is a dangling pointer from entry.c_str() that becomes invalid
after the entry object is destroyed. To fix this, allocate a copy of the string
data on the heap before passing it as user_data, ensuring it remains valid when
the callback is invoked. Then update on_copy_clicked to properly handle and free
this copied string after use to avoid memory leaks.
| CXXFLAGS = -std=c++11 `pkg-config --cflags gtk+-3.0 appindicator3-0.1` -I../include | ||
| LDFLAGS = `pkg-config --libs gtk+-3.0 appindicator3-0.1` -lX11 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add dependency checking and error handling.
The Makefile should verify that required packages are available before building.
-CXXFLAGS = -std=c++11 `pkg-config --cflags gtk+-3.0 appindicator3-0.1` -I../include
-LDFLAGS = `pkg-config --libs gtk+-3.0 appindicator3-0.1` -lX11
+# Check for required packages
+PKG_CHECK := $(shell pkg-config --exists gtk+-3.0 appindicator3-0.1 && echo "ok")
+ifneq ($(PKG_CHECK),ok)
+$(error Required packages not found. Install libgtk-3-dev and libappindicator3-dev)
+endif
+
+CXXFLAGS = -std=c++11 -Wall -Wextra `pkg-config --cflags gtk+-3.0 appindicator3-0.1` -I../include
+LDFLAGS = `pkg-config --libs gtk+-3.0 appindicator3-0.1` -lX11📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CXXFLAGS = -std=c++11 `pkg-config --cflags gtk+-3.0 appindicator3-0.1` -I../include | |
| LDFLAGS = `pkg-config --libs gtk+-3.0 appindicator3-0.1` -lX11 | |
| # Check for required packages | |
| PKG_CHECK := $(shell pkg-config --exists gtk+-3.0 appindicator3-0.1 && echo "ok") | |
| ifneq ($(PKG_CHECK),ok) | |
| $(error Required packages not found. Install libgtk-3-dev and libappindicator3-dev) | |
| endif | |
| CXXFLAGS = -std=c++11 -Wall -Wextra `pkg-config --cflags gtk+-3.0 appindicator3-0.1` -I../include | |
| LDFLAGS = `pkg-config --libs gtk+-3.0 appindicator3-0.1` -lX11 |
🤖 Prompt for AI Agents
In gui/Makefile around lines 2 to 3, the Makefile currently sets compiler and
linker flags without checking if the required packages are installed. Add
dependency checking by using pkg-config with the --exists flag for gtk+-3.0 and
appindicator3-0.1 before setting CXXFLAGS and LDFLAGS. If the packages are not
found, print an error message and stop the build process to prevent build
failures later.
| CXX = g++ | ||
| CXXFLAGS = -std=c++11 `pkg-config --cflags gtk+-3.0 appindicator3-0.1` -I../include | ||
| LDFLAGS = `pkg-config --libs gtk+-3.0 appindicator3-0.1` -lX11 | ||
|
|
||
| SRC = trey_app.cpp ../src/clipboard_mgmt.cpp ../src/db.cpp ../src/logging.cpp | ||
| TARGET = hcp-tray | ||
|
|
||
| all: $(TARGET) | ||
|
|
||
| $(TARGET): $(SRC) | ||
| $(CXX) $(CXXFLAGS) -o $@ $^ $(LDFLAGS) | ||
|
|
||
| clean: | ||
| rm -f $(TARGET) No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing phony targets and improve Makefile robustness.
The static analysis correctly identifies missing phony targets and other Makefile best practices.
CXX = g++
CXXFLAGS = -std=c++11 `pkg-config --cflags gtk+-3.0 appindicator3-0.1` -I../include
LDFLAGS = `pkg-config --libs gtk+-3.0 appindicator3-0.1` -lX11
SRC = trey_app.cpp ../src/clipboard_mgmt.cpp ../src/db.cpp ../src/logging.cpp
TARGET = hcp-tray
+.PHONY: all clean test
+
all: $(TARGET)
$(TARGET): $(SRC)
$(CXX) $(CXXFLAGS) -o $@ $^ $(LDFLAGS)
+test:
+ @echo "No tests defined for GUI component"
+
clean:
rm -f $(TARGET)🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 8-8: Target "all" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
In gui/Makefile lines 1 to 14, the Makefile lacks .PHONY declarations for
targets like 'all' and 'clean', which can cause issues if files with those names
exist. Add a .PHONY target listing 'all' and 'clean' to ensure these are always
treated as commands, not files. This improves robustness and follows Makefile
best practices.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores