Skip to content

feat: convert bookmarks to tags with color highlighting#1296

Merged
karlkleinpaste merged 3 commits into
crosswire:masterfrom
LAfricain:bookmarks2tags
May 2, 2026
Merged

feat: convert bookmarks to tags with color highlighting#1296
karlkleinpaste merged 3 commits into
crosswire:masterfrom
LAfricain:bookmarks2tags

Conversation

@LAfricain
Copy link
Copy Markdown
Contributor

Voici le texte de la PR :


Title: feat: Convert bookmarks to tags with color highlighting

Closes: #968, #685


Summary

This PR converts the existing Bookmarks tab into a Tags system. Tags are a strict superset of bookmarks: every bookmark can be expressed as a tag entry belonging to a named, color-coded folder. This approach resolves both #968 (tags in text) and #685 (color for bookmark folders) in a single coherent feature.

Changes

Data model

  • Added COL_COLOR column (G_TYPE_STRING) to the GtkTreeStore
  • Extended the XML format with an optional color attribute on <Folder> nodes — fully backward compatible (existing files load without modification)
  • Added xml_add_folder_to_parent_colored() and xml_get_folder_color() to xml.c/h

UI — Tag panel

  • New dedicated dialog ui/folder.gtkbuilder (and folder.glade for Windows) with a name field, a GtkColorButton, and a "No color" button
  • "New Folder" and "Edit Item" now open this dialog instead of the generic text dialog
  • A Cairo-rendered color dot (14×14 px, with border) appears to the left of each colored folder in the treeview
  • Drag and drop reordering is enabled by default (no need to activate it from the context menu)
  • Removed the "Set folder color" context menu item — color is now set directly in the folder dialog

Verse highlighting in the Bible text view

  • get_tag_color_for_versekey() in display.cc walks the GtkTreeStore and returns the tag color for the current verse using VerseKey OSIS normalization — this handles locale differences between the stored key format and the display engine format
  • The highlight span includes color: computed from background luminance (ITU-R BT.709) so text remains readable on dark tag colors
  • Multi-reference support:
    • Semicolon-separated refs (Act 16:31; 16:36): book name is propagated to partial refs
    • Comma-separated verses (Eph 2:8,9): expanded to individual verse checks
    • Verse ranges (1 Cor 15:1-4): checked numerically (book + chapter + verse range) to avoid lexicographic comparison bugs

Design decisions

Why disable the current-verse green highlight when a tag color is present?
When a verse has a tag color, the green highlight from settings.versehighlight would override or mix with the tag color, making the tag color invisible on the current verse. We chose to skip the green highlight when tag_color != NULL, so the tag color takes full visual priority. The user can still see which verse is current via the verse number anchor.

Why use VerseKey normalization instead of string comparison?
Stored bookmark keys use locale-dependent formats (e.g. "Actes 16:31" in French) while GTKChapDisp::display() iterates using English OSIS book names ("Act"). Direct string comparison always fails across locales. Normalizing both sides through VerseKey::getOSISBookName() + chapter + verse gives a locale-independent comparison.

Why use numeric comparison for verse ranges instead of OSIS string comparison?
Lexicographic comparison of OSIS refs like "1Co.15.10" vs "1Co.15.4" gives wrong results ("10" < "4" alphabetically). We compare vk->getVerse() directly against vk_start.getVerse() and atoi(dash + 1) as integers.

Why move the color picker out of the context menu?
The context menu item "Set folder color" required a two-step interaction (right-click → menu item → color dialog) and did not work on empty folders. Integrating the color picker directly into the "New Folder" and "Edit Item" dialogs makes it discoverable and works consistently regardless of folder contents.

Localization fix

Default bookmark folder names (Personal, What must I do to be saved?, etc.) were always created in English because xml_new_bookmark_file() was called before gui_init() initialized gettext. Fixed by calling gui_init() before init_bookmarks() in settings.c. gui_init() is idempotent (guarded by a static flag) so multiple calls are safe.

Backward compatibility

  • Existing bookmarks.xml files load without modification — folders without a color attribute simply have no tag color
  • A bookmarks.xml.bak backup is created on first run after upgrade (copied, not moved)

Files changed

  • src/gtk/bookmark_dialog.c — new folder dialog, fixed button handling
  • src/gtk/bookmarks_menu.c — color-aware save, new/edit folder dialogs
  • src/gtk/bookmarks_treeview.c — COL_COLOR, Cairo dot, multi-ref navigation, drag & drop
  • src/gtk/utilities.cutilities_parse_treeview propagates color
  • src/gui/bookmarks_menu.hset_color widget, on_set_tag_color_activate
  • src/gui/bookmarks_treeview.hCOL_COLOR enum, color field in BOOKMARK_DATA
  • src/main/display.cc — tag color lookup, highlighting, luminance-based text color
  • src/main/main.c — backup on first run
  • src/main/settings.cgui_init before init_bookmarks
  • src/main/xml.c/hxml_add_folder_to_parent_colored, xml_get_folder_color
  • ui/CMakeLists.txt — added folder.gtkbuilder and folder.glade
  • ui/folder.gtkbuilder / ui/folder.glade — new folder dialog UI

Please test!

@LAfricain LAfricain force-pushed the bookmarks2tags branch 3 times, most recently from 76067f8 to d6586a8 Compare March 20, 2026 12:51
@karlkleinpaste
Copy link
Copy Markdown
Contributor

I will be looking at this closely in a few days. As I've mentioned, I'm traveling for work this week and have little spare time until I'm home again.

@karlkleinpaste
Copy link
Copy Markdown
Contributor

One immediate observation: ui/folder.gtkbuilder
Do we need an equivalent .glade for windows, or will the windows build use the .gtkbuilder as it stands?

@LAfricain LAfricain force-pushed the bookmarks2tags branch 9 times, most recently from 2c7c9dd to d25230f Compare March 20, 2026 22:15
@LAfricain
Copy link
Copy Markdown
Contributor Author

I need help for Windows, i'm becoming crasy. It's all the more frustrating because these new tags look great on Ubuntu! See what Claude says:

[Windows build] win64: missing webkitgtk-3.0 for mingw64, and folder/edit bookmark dialogs not working

Context:
We are working on the bookmarks2tags branch which adds color tagging to bookmark folders. This required a new folder.gtkbuilder (GTK3) and folder.glade (GTK2) dialog.

Problem 1 — win64 build fails:
The current win64 build uses -DGTK2=ON for both win32 and win64. We changed win64 to use GTK3 (removing -DGTK2=ON) so that folder.gtkbuilder and the languages file get packaged correctly by CPack. However, mingw64 only has webkitgtk-1.0 (mingw64-webkitgtk), not webkitgtk-3.0 or webkit2gtk. This means GTK3 win64 can't find a webkit dependency.

We tried -DGTKTVEDITOR=ON to bypass webkit, but this breaks Linux builds because XIPHOS_HTML_* macros are used unconditionally throughout the codebase.

Question: What is the recommended way to build win64 with GTK3 without webkit? Is there a mingw64 webkit package available, or should we protect all XIPHOS_HTML_* usages with #ifndef USE_GTKTVeditor?

Problem 2 — folder/edit bookmark dialogs do nothing on Windows:
Clicking "New folder" or "Edit bookmark" in the bookmarks panel does nothing on Windows. Root cause: folder.gtkbuilder and the languages file are not installed because ui/CMakeLists.txt excludes them when GTK2=ON:

if (NOT GTK2)
    install (FILES folder.gtkbuilder ... languages ...)

This is directly tied to Problem 1 — fixing the win64 GTK3 build would resolve both issues.

@LAfricain
Copy link
Copy Markdown
Contributor Author

LAfricain commented Apr 3, 2026

Ok, some updates. I got the build working on Windows, and I got the languages working too (no more unknown)! Thank God (and Gemini for this time). The bookmarks work almost:

  • In Windows, I can create new bookmarks, and the references open just fine (single, multiple, etc.).
  • The only problem is that I don't know how to create a new folder. The window opens, I can choose a color, but when I close it, I don't see a folder.
  • I still need to test the right-click tool on the Bible text to add a bookmark.

I imagine the problem lies in folder.glade. If anyone is willing to help me finish the job, that would be great.

@LAfricain LAfricain force-pushed the bookmarks2tags branch 3 times, most recently from 5031f73 to c4a4212 Compare April 4, 2026 11:08
@LAfricain
Copy link
Copy Markdown
Contributor Author

LAfricain commented Apr 4, 2026

I'm happy to announce that it works! Please try it out for yourselves. Now I can create folders on windows, and the colors are applied to the verses. The color appear in front of the tag like it does in Linux.

@LAfricain LAfricain force-pushed the bookmarks2tags branch 8 times, most recently from 95ca928 to a29c4b8 Compare April 6, 2026 11:56
@LAfricain
Copy link
Copy Markdown
Contributor Author

I'm really flattered that you're asking me—a total newbie who doesn't know anything about this—how to get my branch! But I pushed the branch to my personal fork and opened a Pull Request for it. You can see all the code changes directly here in the PR: PR 1296

If you want to pull my branch locally to test the code, you can fetch it directly from the PR using this command (you probably know this, but I say it anyways!!):
git fetch origin pull/1296/head:bookmarks2tags
git checkout bookmarks2tags

(Or if you use GitHub CLI: gh pr checkout 1296)

The branch in my fork: https://github.com/LAfricain/xiphos/tree/refs/heads/bookmarks2tags

If I need to do anything else, let me know.

@LAfricain
Copy link
Copy Markdown
Contributor Author

@karlkleinpaste did you found a way to my branch?

lafricain79 added 2 commits May 1, 2026 21:14
- Add COL_COLOR column to bookmark tree store (bookmarks_treeview.h/c)
- Add color attribute to XML folder nodes (xml.c/h)
- New folder dialog with color picker (ui/folder.gtkbuilder, ui/folder.glade)
- Tag color saved and restored from bookmarks.xml
- Verse highlighting in Bible text using tag color (display.cc)
- Text color auto-adjusted for dark backgrounds (luminance)
- Multi-reference support: semicolons, commas, verse ranges
- Popup menu for multi-reference bookmarks navigation
- Drag and drop reordering enabled by default
- Cairo color dot in treeview for tagged folders
- Current verse green highlight disabled when tag color present
- Fix bookmark folder labels localization: gettext now initialized
  before default bookmarks are created, so folder names (Personal,
  What must I do to be saved?, etc.) are properly translated on
  first run

Closes crosswire#968
Closes crosswire#685
@karlkleinpaste
Copy link
Copy Markdown
Contributor

I had not found opportunity to look at this in the last week, though I did take care of the ReadAloud PR and made a couple updates past it. ReadAloud from mouse selection now works properly ... in Linux. The builds fail in Windows.

I was also not aware of the means to get at another's private branch until you mentioned it -- I am not a heavy user of GH and freely admit to being ignorant of many of its capabilities.

I see you've just pushed an update. I gather from this push that you must have rebased to master; true? Then you'll see that the Windows build problems need to be handled. At first glance from the failed log, I think there is just some *.h file in need of being included, so as to properly declare a series of required functions, but I am not sure. I know very little of these Windows interfaces.

@LAfricain
Copy link
Copy Markdown
Contributor Author

The crash occurs specifically when using right-click on Bible text → Add bookmark, but not when using the bookmark panel directly.

The backtrace pointed to strlen() receiving an invalid address (0x00631035004C1858), called from GLib via gtk_tree_store_set().

After analysis, the root cause is in add_bookmark_button() in bookmark_dialog.c:

// BEFORE — uninitialized memory, data->color is never assigned
data = g_new(BOOKMARK_DATA, 1);
...
// data->color is never set → random garbage value

gtk_tree_store_set() then passes data->color (a random pointer) to GLib which calls strlen() on it → segfault on Windows.

The other path (on_add_bookmark_activate() in bookmarks_menu.c) works fine because it explicitly sets data->color = NULL.

The probable fix:

// AFTER — zero-initialized, data->color is NULL by default
data = g_new0(BOOKMARK_DATA, 1);
...
data->color = NULL;  /* bookmark leaves never have a color */

This works on Linux by luck — the kernel tends to return zeroed pages, so data->color happens to be NULL. On Windows, uninitialized heap memory contains garbage values, hence the crash.

But the Windows build failure on utilities.c is probably unrelated to this PR. It originates from commits de8f4395 and e8daa21f (PR #1307) which introduced Windows SAPI/TTS support using C++ headers (sphelper.h) compiled as C by MinGW-w64, causing:

  • Default arguments in sphelper.h (C++ syntax, invalid in C)
  • CoCreateInstance called with CLSID/IID by value instead of by pointer
  • SPF_PURGEBEFORE undeclared (should be SPF_PURGEBEFORESPEAK)
  • COM virtual methods (->Speak(), ->Release()) inaccessible in C compilation

This PR only modifies bookmark_dialog.c, bookmarks_menu.c and related bookmark files. utilities.c is untouched.

@karlkleinpaste, can you have a look about the CI?

@LAfricain
Copy link
Copy Markdown
Contributor Author

Oops, our messages crossed paths—almost at the exact same moment!
But yeah, I base it, and sure enough, it no longer compiles on Windows! Okay, I'll leave that to you. I really hope this time it's the solution to the bookmarks problem (I'm speaking about the new commit for this PR)! If it is, Claude will have it fixed in less than 10 minutes.

@karlkleinpaste
Copy link
Copy Markdown
Contributor

You're asking a lot. I'm not kidding.

The problem presented by your (Claude's?) new TTS code for Windows is that it's trying to use C++ in a C environment. When I ask the AI for suggestions, it says bluntly, "Disable TTS for Windows build" with assorted #ifdef/else/endif bracketing to protect the new Linux code and cancel Windows TTS outright. Frankly, I could be convinced to do just that, in the name of moving forward.

I said I know next to nothing about Windows interfaces like this. I Don't Do Windows.

@karlkleinpaste
Copy link
Copy Markdown
Contributor

I just disabled Windows TTS entirely for the moment.
The Ubuntu build failed but it's obviously because of repository problems at GH.

Oh for pity's sake.
/usr/lib/gcc/x86_64-w64-mingw32/8.3.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -lspeechd
This is caused by -lspeechd in src/gtk/CMakeLists.txt as an unconditional library addition that is not available in Windows.
Use of -lspeechd needs to be conditional on Linux builds only.
I'm adding another change to move it to an else after if WIN32.

The entire Windows speech re-work has been very poorly prepared.

@karlkleinpaste
Copy link
Copy Markdown
Contributor

All right... Windows TTS is disabled and Windows builds succeed.
Tomorrow (it's getting late here) I will try to build Windows with your bookmarks code.
Or you can force a GH build yourself.

@LAfricain
Copy link
Copy Markdown
Contributor Author

@karlkleinpaste Sorry for asking so much—to be honest, I don't always realize it! Sorry also for the poor work on the TTS issue; I didn't do it with Claude but with DS and Gemini. Their level is nowhere near Claude's.
I just want to remind you of what we’ve accomplished in just a few weeks! We’ve reached for the moon, as we say in French... What I mean is, I was able to do it without any coding skills, so I don’t doubt for a second that you, with your skills and working with Claude, can do much more.
I’ve also realized, since I’ve gotten my hands dirty, just how time-consuming coding is... So thank you for all those hours you’ve given for the glory of God. Let’s also pray that the Lord will send us collaborators, especially those who know their way around Windows.
About forcing windows to build by myself, I don't know how to do this, and I think I can't. I don't have the right to do it.

@karlkleinpaste
Copy link
Copy Markdown
Contributor

"...if you have write access..."

I don't have that kind of access to your branch.

screenshot4

@karlkleinpaste
Copy link
Copy Markdown
Contributor

Success! xiphos-4.3.2.56-win64.exe: Can add bookmarks without crash.

@karlkleinpaste karlkleinpaste merged commit efbd7be into crosswire:master May 2, 2026
7 checks passed
@LAfricain
Copy link
Copy Markdown
Contributor Author

LAfricain commented May 2, 2026

😊 😊 😊 😊
Can you imagine (see all the history) with deepseek and Gemini hours of search... With Claude 10 minutes!!!!!
Finally we succeeded. And what do you think about the feature?

@LAfricain LAfricain deleted the bookmarks2tags branch May 2, 2026 17:59
@karlkleinpaste
Copy link
Copy Markdown
Contributor

what do you think about the feature?

I am ... mystified. I see no difference in how folders are handled. New Folder gives me the usual, simple 1-line "give it a name" prompt. The UI commentary at the top says:

"New Folder" and "Edit Item" now open this dialog instead of the generic text dialog

Not so:

image

So what am I missing?

folder.gtkbuilder is present in /usr/share/xiphos.

@karlkleinpaste
Copy link
Copy Markdown
Contributor

Never mind. Old instance running where I thought I had restarted it.

Got it now.

@karlkleinpaste
Copy link
Copy Markdown
Contributor

OK, serious, serious, serious, SERIOUS UI interpretation problems.

I have lots of bookmarks that are multi-verse sets. Prior to this, when I would click them, Xiphos would dump them into the Verse List as a set of individual references.

Now I'm getting all sorts of weird behaviors. And I do mean, all sorts.

I have edited a folder to green. I have a bookmark in that folder to Isaiah 4:1. Now the entire chapter is green.

screenshot3

No, no, no, no, no.

But this doesn't occur all the time. Just some of the time.

A multi-verse bookmark containing some 2 dozen entries is now offered as a pull-down, not as a dump into the verse list.

screenshot5

Not acceptable.

Multi-verse bookmarks should still dump their contents into the verse list, the way they used to, so that I can then click them individually as I see fit. Changing the semantic of a multi-verse bookmark to a pulldown selection of one entry within it is simply not correct.

But sometimes, multi-verse bookmarks still do dump into the verse list.

<Bookmark modulename="" key="Matthew 15:6; Mark 7:9; Matthew 15:1-9; Mark 7:1-13" moduledescription=" " description="tradition nullifies the word"/>

This leads to:

screenshot7

In that listing, I clicked the middle entry: The entire chapter of Mark 7 is green. On second thought, no, not the entire chapter. Large chunks of the chapter, then ordinary black-on-white text, then a personal annotation I've got in v.15, then a paragraph of black-on-white, then 2 big paragraphs of green. And what is that red block before my v.15 annotation?

screenshot8

I have other Bibles in nearby tabs. The display is not even consistent across them. BSB lacks the green from v.24 onward. Same for ESV and NASB. I have a NIV module (not redistributable) which displays almost like BSB, ESV, and NASB, but it also turns green just verses 24-26 and 31-32.

A thousand times, no.

Abruptly, this has become a UI disaster.

The inconsistency of all this is deeply disturbing. I do not like this, not at all, not one bit. I'm already at the edge of reverting it. The entire existing UI interpretation and semantic of "click a bookmark" should not have changed just by adding color to bookmark folders.

@karlkleinpaste
Copy link
Copy Markdown
Contributor

I just realized after the fact that my example bookmark above that begins "Matthew 15:6" did not dump the entire set of verses into the verse list. It dumped only 3.

I will be reverting this PR momentarily.

karlkleinpaste added a commit that referenced this pull request May 2, 2026
@LAfricain
Copy link
Copy Markdown
Contributor Author

All these issues you're having are really strange. It sounds more like a failed migration. I didn't have any tags before, and I'm not experiencing any of the problems you're describing. The colors are displaying correctly. And under Wine too, based on the quick tests I just ran.
Normally, there was supposed to be a process for migrating the old tags, but apparently it isn't thorough enough.
I can't help you figure out what's going on!

@LAfricain
Copy link
Copy Markdown
Contributor Author

I added your entry to my bookmarks and I'm having the same problem as you. It's true that I was tempted to do it differently. I create a folder with a theme and a color, then add my bookmarks to it one by one as I go.
But even so, I created two bookmarks with multiple links, and the links open just fine in the context menu.

bookmarks.xml

@karlkleinpaste
Copy link
Copy Markdown
Contributor

For the moment, I suggest re-introduction of the code in some piecemeal fashion, ensuring that all of it is in place exactly as intended. The chapter excess colorization rendering is most concerning, as is the new UI interpretation of clicking a multi-verse bookmark. A multi-verse bookmark should have its contents dumped into the verse list, period -- no change from ordinary behavior up to now. To turn that into a pulldown means there's a lot of code in there to handle that deliberately. That's not a mistake and it's not a migration error, that's a considered, deliberate change. And to have a multi-verse bookmark cut short when dumped into the verse list is plain wrong -- getting that wrong means that the handling of the expansion of the textual content of the bookmark into a bookmark list (internally, a "GList *" set) was badly damaged.

@LAfricain
Copy link
Copy Markdown
Contributor Author

Can you send me a copy of your bookmarks file? I'd like to try migrating with them on Monday. The weird thing is that if I create a label with a title and add your references to it, everything works perfectly! The references open in the context menu, and the verses are displayed in the correct colors...

@LAfricain
Copy link
Copy Markdown
Contributor Author

Karl, you sound angry. And I don't think that's justified. We're all doing the best we can :)

A multi-verse bookmark should have its contents dumped into the verse list, period

I think you’re overreacting here. It’s really weird to click on a bookmark and—poof—it opens in another tab, and then you have to go back to the other tab. I’m the one who wanted this behavior because I find it more consistent (and I think I described it). Is it at least possible to discuss this?
I didn’t have any bookmarks because I didn’t find them appealing before. So I couldn’t have imagined there would be this kind of problem.
So I’ll let you decide what works and what doesn’t, but don’t be unfair—if it weren’t for that terrible migration issue, we’d have a great feature here.

@LAfricain
Copy link
Copy Markdown
Contributor Author

LAfricain commented May 3, 2026

Dear Karl,
I hope you've gotten over the bad experience with the bookmarks. I took a closer look at them this morning. And actually, it seems to me that the situation isn't as bad as all that. (From what little I could see in the bookmarks you shared.)
I’m not talking about the issue of opening multiple verses in the bookmark window or in a context menu; I’m talking about the highlighting.
I actually see that you have four references in the bookmark you shared: Matthew 15:6; Mark 7:9; Matthew 15:1-9; Mark 7:1-13
However, two of them are actually duplicates (Matthew 15:6 is included in Matthew 15:1-9, and the same goes for Mark). So it’s normal for the highlighting to cover all these verses. That leaves the mystery of verses 24 through 31 to figure out. But maybe you have another bookmark with that series?
If so, then everything is in order. If you don’t like the highlighting, it’s not mandatory—you can remove it. That’s the beauty of it: we can do whatever we want. Now, we could also add an option to have a color just for the bookmark that doesn’t apply to the verses, and another option that highlights the verse—personally, I’m not sure it’s worth the effort.

Now, about the context menu for the verses. I understand that you don’t see things the same way I do. And it’s up to you. I’m willing to do sacrifice this feature. But please think it over first. A context menu that pops up is super convenient; it saves you from clicking all over the place to open a reference (without the context menu, you might end up constantly switching between different windows). With the context menu, you have everything right at your fingertips, so to speak. But anyway, I see that the context menu doesn’t always work. When there are ranges in the references, it opens everything in the verse window. I probably have the solution, but it all depends on what you decide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants