Fix markup parsing error for paths with '&' in places sidebar #1819
Conversation
|
Note: The Travis CI build (#275081104) fails in the Fedora environment with the error No package 'libtool-ltdl-devel' found. This issue appears related to the Travis CI configuration (.travis.yml) or build scripts, not the changes in this PR. The build was successful locally on Ubuntu 24.04.2, as noted in the PR description. Could the maintainers review the Fedora dependencies? |
Package still exist in fedora |
|
Thanks for confirming that libtool-ltdl-devel exists in Fedora (Koji #2641628). The Travis CI build (Job #632592193) fails in Fedora with No package 'libtool-ltdl-devel' found and in Debian with ./docker-build --name ${DISTRO} --config .build.yml --install exiting with code 1. Both issues seem related to the Travis CI configuration (.travis.yml or .build.yml) or build scripts, not this PR's changes. The build succeeded locally on Ubuntu 24.04.2, as noted in the PR description. I also noticed a Gtk-CRITICAL error in my local logs for mate-panel (gtk_widget_destroy: assertion 'GTK_IS_WIDGET (widget)' failed). This appears unrelated to this PR, as it occurs in a different module and does not affect Caja's functionality. Could the maintainers review the Fedora and Debian setups in Travis CI? Please let me know if further clarification or testing is needed! |
|
I will definately need others on the team to help with that. I am not on Fedora
locally (and don't have a landline to download installers over) and don't have
a lot of understanding of how to fix travis problems.
…On 5/4/2025 at 5:14 AM, "Bernd Binder" ***@***.***> wrote:
banad60 left a comment (mate-desktop/caja#1819)
Thanks for the feedback and for confirming that libtool-ltdl-devel
exists in Fedora (Koji #2641628). The Travis CI build (Build
#275081104) failure in Fedora with No package 'libtool-ltdl-devel'
found seems to be a configuration issue in .travis.yml or the
build scripts, unrelated to this PR's changes. The build succeeded
locally on Ubuntu 24.04.2, as noted in the PR description.
Additionally, I noticed a Gtk-CRITICAL error in my local logs for
mate-panel (gtk_widget_destroy: assertion 'GTK_IS_WIDGET (widget)'
failed). This appears unrelated to this PR, as it occurs in a
different module and does not affect Caja's functionality with
this fix. Could the maintainers review the Fedora setup in Travis
CI?
Let me know if further clarification or testing is needed!
--
Reply to this email directly or view it on GitHub:
#1819 (comment)-
2849012356
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
|
|
Thanks for the update, @lukefromdc! I understand the Travis CI issues (Fedora and Debian) are tricky and appreciate the team looking into them. My local build on Ubuntu 24.04.2 works fine, as noted in the PR. Happy to run additional tests or provide more details if needed. Let me know how I can assist! |
|
Just confirmed the error message |
There was a problem hiding this comment.
The code works, but why are you cutting out so many existing comments?
We need to keep those for maintainability in the future. Bring back the comments
and this should be ready to go. No build problems on Debian here, and it fixed both
the error in .xsession-errors and the wrong tooltip text
|
Thank you for confirming the bug and pointing out the issue with the removed comments. I agree that keeping the code well-documented is important, so I’ll work on restoring the comments to make everything clear again. I’ll update the pull request with the restored comments in the next few days. It might take a little time, but I’ll let you know once it’s ready. Thanks for your patience and support! |
|
Thanks, it gets here when it gets here
|
|
It's finally here |
lukefromdc
left a comment
lukefromdc
left a comment
There was a problem hiding this comment.
Just ran into a new problem and confirmed it happens only with this PR applied. If I right-click on any item in the places sidebar, the context menu comes up with all items greyed out and is unreponsive. Even to close it sometimes requires closing the entire window.
Left-click works as expected, setting the directory for the open window to the clicked item, Middle click opens it in a new tab, but right click fails.
Tested in the wayland session but this probably will also happen in the x11 session or at least the 100% greyed out entries in the context menu would be expected.
The correct filename of the test folder is read when hovering over it so that part works
Making this a comment not a formal request for changes in case anything happens to me with the current mess in the US
|
Holding on the github-actions workflow approval as we already know we have a problem that needs to be fixed |
|
I can also confirm the error in the right-click menu under X11 with LightDM: gray, non-navigable menu items. I'll investigate this; it's probably related to the first commit. |
cwendling
left a comment
cwendling
left a comment
There was a problem hiding this comment.
This is not reviewable. Don't include seemingly random changes in a pull request.
The meaningful change here seem to simply be the g_markup_escape_text() call (which probably make sense indeed), the rest might or might not be interesting but it's definitely not related and must not be part of the same PR. Also, such changes need be explained in the commit message.
| if (tooltip) { | ||
| escaped_tooltip = g_markup_escape_text (tooltip, -1); | ||
| } else { | ||
| escaped_tooltip = g_markup_escape_text ("", -1); |
There was a problem hiding this comment.
No need to escape a string we know is empty and thus has nothing to escape, especially when a NULL value is fine (it's either OK or this code path is never used because of the if (tooltip) clause above).
| escaped_tooltip = g_markup_escape_text ("", -1); | |
| escaped_tooltip = NULL; |
I'd suggest to also follow style and put braces on their own lines. Also, if you initialize escaped_tooltip = NULL` the else clause is unnecessary.
| if (escaped_tooltip != NULL) | ||
| { | ||
| g_free (escaped_tooltip); | ||
| } |
There was a problem hiding this comment.
No need for guarding against NULL, g_free() (like free()) is NULL-safe.
| if (escaped_tooltip != NULL) | |
| { | |
| g_free (escaped_tooltip); | |
| } | |
| g_free (escaped_tooltip); |
(also note that without my suggested change above, escaped_tooltip just cannot be NULL anyway)
|
It looks like it's working! Tested with Caja 1.26.4. |
|
We need to remove the style changes from this (they can be for a separate PR) and we need to fix that content menu greyout issue before this can proceed |
5 participants
Fix for Markup Parsing Error with '&' in Paths in Caja
Problem
Caja displays a GTK warning in ~/.xsession-errors when bookmark paths contain special characters like & (e.g., /home/user/WORX/MA&PA). This causes a markup parsing error in the "Places" sidebar.
Reproduction
Solution
In src/caja-places-sidebar.c, the tooltip text in the add_place function is escaped with g_markup_escape_text before being inserted into the GtkListStore. This prevents parsing errors caused by special characters like &.
Changes
src/caja-places-sidebar.c: Escape tooltip text with g_markup_escape_text.
bookmarks_check_popup_sensitivity: Added check for NULL popup menu to avoid Gtk-CRITICAL error.
Tests
Locally tested on Ubuntu 24.04.2 with Caja 1.29.0.
Added bookmark with &, no GTK warnings in ~/.xsession-errors.
Known Issues
Xlib errors (ignoring invalid extension event 161 and 146) occur with tooltips. These are non-disruptive and likely related to the compositing manager (e.g., picom). They can be investigated later.
Reference
Relates to Issue #1549.