Bookmark color solves #685

[LAfricain]

As suggested, I've separated the various changes from the previous PR, which included folder colors, highlighting, and the context menu for crossreferences. Here you'll find only:

• Folder colors
• Drag-and-drop for bookmarks

However, I'm sorry about those two commits—I made a mistake without realizing it.
I tested the Windows binary, and it works.

[karlkleinpaste]

Testing the patch.

After creating a new bookmark folder, and having given it a color, there is no way to get rid of its color. The No color button has no effect. I can change color successfully but I can't get rid of colorization.

[karlkleinpaste]

Also, remove cmake/source_version.txt from the patch.
git checkout -- cmake/source_version.txt

[LAfricain]

Okay, I'll take a look at that tomorrow. Good job figuring that out! I hadn't even thought of that.

[LAfricain]

I finally did it. It works for me. I also took the opportunity to merge the two commits so it would be clean.

[LAfricain]

As for the other commits that gave you trouble, namely, highlighting verses based on the color assigned to the bookmark and the issue of using a dropdown menu instead of a toggle for the verse field, I have a suggestion. Why not suggest a checkbox "highlight the verses" and "display the references in a drop-down menu"? Where would be the best place to discuss this?

Or, if you agree with the highlighted text, I can try to figure out what the problem is, but it would be easier with a file that looks like yours.

[karlkleinpaste]

Turning off color seems to work now.

I'm looking through the code very slowly and carefully. A number of things are a source of concern.

• There is a great deal of mere whitespace change. One obvious example: Diff in add_bookmark_button would be very small if the left margin whitespace hadn't been gratuitously changed.

A core software principle of mine is "small footprints": The smallest amount of change to achieve the intended result. Changes of this sort -- just whitespace -- create far larger diffs for no reason, losing the visibility of the actual functional change, making code review hard and less reliable. Somebody else has to read this code (at the moment, me), and making others' eyes get lost in useless change is not helpful. Did the actual, functional, running code in these lines change, or is it just re-indentation with no effect on operation? Is there perhaps a single function argument that changed somewhere in the middle of 3 dozen lines of otherwise-unmodified-but-differently-indented code? How likely is it that someone else will miss such a change?

• The content of function bookmark_dialog.c:on_dialog_response has been removed, though the function itself remains -- it's a NOP. This function is used in a number of places as a callback function. This cannot afford to disappear. Why was this done?

Of course, now that I look more carefully, I find that there are several on_dialog_response functions across a number of files. Much of this is very old code, and much of it is not mine (Xiphos [neé GnomeSword] was first created in 2000), and I don't have a reason to cite for why multiple instances of such a function are needed. Nonetheless my concern about eliminating function content remains: If this one instance of on_dialog_response is unneeded, then it should go, not merely be castrated, lest something else that actually needs its intended utility try to use it and unavoidably fail.

I'm still working through these diffs. It's going to take some time in the current state of things.

[LAfricain]

Claude helped me to fix the issues you pointed out. The spaces have been replaced with tabs, and the function that had been deleted (I don't know why) has been restored because it's used elsewhere.

[karlkleinpaste]

I've had to put off looking closely at this patch for a bit. I got to it today.

The amount of gratuitous whitespace change is insufferable. I'm not kidding.

Consider changes in bookmark_dialog.c. Only in bookmark_dialog.c.

That's just bookmark_dialog.c. Almost half this single file's diff is caused by whitespace changes.

I don't like coming off as something of a jerk about this, but I can't help it. Small footprints. The diff for bookmark_dialog.c alone would be barely half the size it needs to be, if unchanging code would be left alone.

Doing a review on something like this means I have to go through these sorts of changes to see if there's any actual different coding content. I can't help but be suspicious when I encounter this. Was there really a change to storage of osisrefmarkedverses, or am I missing some critical mis-spelling of a necessary keyword that's been corrected, or are there arguments previously out of order that have been corrected? Oh... Well, no. The textual change is a functional nullity.

I would really like to get bookmark color into Xiphos, and I appreciate the effort. But clean effort is required. Not just automated effort by Claude. If Claude is doing this, then Claude is doing stupid stuff.

[LAfricain]

Okay, Karl, give me some time to look into this next week.

[karlkleinpaste]

Vastly better, and thank you. There remain some significant peculiarities.

There is no way -- absolutely no way -- that this code builds successfully, and I don't care if GitHub says "all checks have passed." Maybe that observation was for the first time around, not for recent edits, because this cannot build:

There are other oddnesses of this sort:

A curiosity: Why did initialization order need to change?

This is close, and I very much want to merge this. But I need you to take another look to resolve these questions. I'll try to be more prompt about updates.

[LAfricain]

For the two errors about 'sed -n' in the files, I did already the corrections. Are you sure about it? I have a second look in the tab "files changes". I can't find it.

[LAfricain]

I was wrong—the corrupted parts were still there, but in the comments. The three errors have been corrected.

[karlkleinpaste]

I'm now experimenting with the code, and much is fine. One thing is not: New bookmark folders are given a default of black color. Should it not be preferred that a new, otherwise unremarked folder be given No Color? That would fit with the (naturally historical, until this PR) habit that Xiphos' bookmark folders do not have color. Folders should get a color attached when the user actively chooses it, not tacked on as a default.

Editing a folder to use No Color works, yay.

[LAfricain]

Summary of changes addressing your feedback:
Default color behavior:

• New bookmark folders now have no color by default, as you requested
  The color button is always active, but color is only saved when the user explicitly chooses one
• Clicking the color button directly now automatically sets the state to "No color" (meaning a color is active), making the workflow more intuitive
• The "Add color" / "No color" toggle button clearly indicates whether a color will be saved
• Editing an existing folder without color correctly shows "Add color"
• Editing an existing folder with color correctly shows "No color" and pre-loads the existing color

[karlkleinpaste]

Thanx much for the extra effort.