Skip to content

Bookmark color solves #685#1317

Open
LAfricain wants to merge 1 commit into
crosswire:masterfrom
LAfricain:bookmark-color
Open

Bookmark color solves #685#1317
LAfricain wants to merge 1 commit into
crosswire:masterfrom
LAfricain:bookmark-color

Conversation

@LAfricain
Copy link
Copy Markdown
Contributor

@LAfricain LAfricain commented May 28, 2026

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

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

@LAfricain
Copy link
Copy Markdown
Contributor Author

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

@LAfricain
Copy link
Copy Markdown
Contributor Author

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

@LAfricain
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

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.

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