Add color highlight menu

[smasher816]

Pass a configurable ColorRGB32 to bb:paintRect / bb:lightenRect.
If isColorEnabled is unset then original B&W Color8/uint8_t is passed in.

Requires corresponding Cbb patch.
koreader/koreader-base#1680

---

This change is 

[smasher816]

Initial inspiration. #9024 (comment)

[hius07]

Since the color is constant within a page, it can be calculated outside of the loop on each box.

[hius07]

people will want different color per highlight

In view of this in the future, the general setting should be done as a submenu, not RadioButtonWidget, similar to highlight style.
RadioButtonWidget would be used in a certain highlight style editing (personal style would include drawer, color and maybe opacity).

[hius07]

BTW, we do not have a feature "apply default style to all highlights in the book", it might be useful.

[ryanwwest]

Initial inspiration. #9024 (comment)

This is great. I was wondering if the highlight colors could possibly match the default hex colors offered in Zotero, if they aren't finalized? It looks like what you currently have is already very close and that issue also mentions Zotero. I'd like to at some point develop some sort of sync between Zotero highlights and KOReader highlights so you can read/write them in either app and this would solve one of the big issues listed here. The colors seem to be these.

[smasher816]

Newest revision now supports per-highlight colors. This extra info should be part of highlight exports, thus making it queryable to outside integrations.
Settings menu is disabled if color screen is not present.

@hius07 I fail to see how the radio menu is bad. It seems to work fine for both setting the default, and for editing a single highlight.
Apply default style to all highlights can be a future MR.

@ryanwwest the color values came from the Onyx drawing app. These are pretty finicky in that certain colors appear identical (I had to manually tweak orange to not be the same as yellow). I suspect that really these may need to be tweaked per-device.
I think it might be a good idea to make the list of colors configurable using a table in settings.lua, but I'm unsure how much work that would be. Edits to the config would likely have to be manual (or through the advance screen) since there is no easy way to edit these values. Regardless, I think this can be a future addition to avoid feature creep.
For the Zotero integration, I don't think it would be tough to have a lookup table / translation layer to accomplish your goal.

[smasher816]

Latest revision allows for customizable colors in settings.lua
(this one is for you @ryanwwest )

    ["highlight_colors"] = {
        [1] = {
            [1] = "Rojo",
            [2] = "#fe4400",
        },
        [2] = {
            [1] = "Naranja",
            [2] = "#ff8800",
        },
        [3] = {
            [1] = "Amarillo",
            [2] = "#fdff32",
        },
        [4] = {
            [1] = "Verde",
            [2] = "#00ad65",
        },
        [5] = {
            [1] = "Morada",
            [2] = "#ee00ff",
        },
        [6] = {
            [1] = "Gris Oscuro",
            [2] = "#303030",
        },
    },

Edit color button is now completely removed on non-color devices.

[smasher816]

Newest revision should now pass in pdf annotation colors to mupdf backend.
Base MR has also been updated to receive this value.

[ryanwwest]

Latest revision allows for customizable colors in settings.lua (this one is for you @ryanwwest )

Thanks. FYI, the way Zotero allows highlight colors is it directly stores (with the highlight) any color hex code and displays that color. But the UI color picker that shows up when you select text to highlight only has like 8 or 9 preset colors, so users don't usually see other colors. I'm not sure if that would also work here (not looking super closely at the code) but if not it might be okay. As a future annotation syncer plugin with Zotero could instead create a new highlight_colors entry with each incoming zotero color hex code not already found in the table.

[smasher816]

Custom colors are now with patches.
See #11044 (comment)

[poire-z]

Should we really save the rgbcolor (#FF0000) in the highlight item?
Wouldn't it be cleaner/more abstract if we save keywords (red, green...) - so the change-color-dialog shows always something checked - and we can change what color is "red" and have it apply to all past "red" highlights ?

[ryanwwest]

Should we really save the rgbcolor (#FF0000) in the highlight item? Wouldn't it be cleaner/more abstract if we save keywords (red, green...) - so the change-color-dialog shows always something checked - and we can change what color is "red" and have it apply to all past "red" highlights ?

I'd prefer the rgbcolor itself to be saved into each highlight item as then it allows any number of highlight colors (and also easier for future sync with Zotero). It seems confusing to me to change the color of 'red' to be a non-red color (slight shades of red variation makes more sense but I'm not thinking of a common use case for this).

[poire-z]

change the color of 'red' to be a non-red color (slight shades of red variation makes more sense but I'm not thinking of a common use case for this).

I was thinking about the "slight shades of red" case, just for people wanting to adjust the red saturation/tone to make it more readable to them on their devices (color eInk or Android) when set on a black or blue or grey text - they may want to make it lighter or darker when they later meet greyish text in books and realize that initial red is not working well in some cases.

I don't think people are so in love with red that they will want to override our blue to be another kind of lighter red than our red, and our yellow to be another variation of
https://www.color-meanings.com/shades-of-red-color-names-html-hex-rgb-codes/
(but I may be wrong :)

[ryanwwest]

Well you could have both - hex codes to allow any color, and some preset red/green/yellow string which is mapped to a configurable hex code. I see benefits for both of them..

[ryanwwest]

I am not sure how this fits in with the existing highlight styles. is it just a new field, separate and in addition to underline/strikeout/invert/highlight, or do you have a color in place of those? I'm indifferent to either option, though I don't know if invert fits with colors.

[poire-z]

I'm not thinking so much about users of external tools, just our internal code - and it feels keywords would allow more future features than harcoded color codes - ie what we do with underline/striken, one could in the bookmark list show all "red" highlights only, if for that user, "red" means "typo error to correct", etc...

[smasher816]

Saving the name still allows for "any number of highlight colors". I don't see any downside to doing that, and having the ability to easily patch old highlights seems useful to me.
And ryam, it'. separate field. You can still highlight/underline/strikeout with the selected color. It just changes how it is shown.
Invert has no color option, since it flips whatever is already drawn on the page.

[smasher816]

ReaderHighlight now stores string names, and ReaderView now has a name->rgb lookup table.
There are fallbacks if a lookup fails (someone may add a new name in a patch which is then later disabled).

[NiLuJe]

Yeah, I suppose the color names are sort of an artifact of the fact that the color values used to be in ReaderHighlight, too, at some point.

In any case, since the values in BB are patchable, I suppose it makes sense to leave the names patchable, too ;).

[NiLuJe]

(row of 5 + row of 4 looks a bit ugly. Fortunately, people with non-color devices will continue to get quality UI :)

@poire-z: Another idea: Offloading the style/color buttons in an anchored ButtonDialog from a single "highlight" button ? That would mean an extra tap to access those (but then again, how often does someone do that?).

It would allow unlocking a bunch of the stuff on !Color screens, which could make sense if documents (esp. PDFs, I imagine) are shared with color devices?

[poire-z]

You mean we would have:
Delete | Highlight | Add note | ...
and on tap on Highlight some popup would happen with Style and Color stuff ?

No real opposition for me as I don't use these - and fine if it keeps this dialog 2x4 buttons.
Except that Highlight there sounds too generic (after all, we just tapped on a highlight :), so it could/should just be named Style.

(@hius07 did the Style radio button widget, so he should have more to say about that than me.)

[hius07]

RadioButton widget can show optional additional ("extra") button below Cancel-OK row.
We can keep the "Style" button in the popup dialog as is, and show extra "Color" button in the "Highlight style" dialog for color devices.

[NiLuJe]

RadioButton widget can show optional additional ("extra") button below Cancel-OK row. We can keep the "Style" button in the popup dialog as is, and show extra "Color" button in the "Highlight style" dialog for color devices.

Oh, yeah, that's much simpler, I like it.

[NiLuJe]

Another random query: should we keep the fancy colored backgrounds in the color picker on grayscale devices?

I suspect it'll look worse on actual eInk screens because of quantization, making contrast a potential issue. Then again, that is actually how they will render, so it's sort of a good indication that "huh, maybe I should not be using that color?" ;D.

[NiLuJe]

Oh, and because grayscale reminds me, I mentioned earlier that I'd fixed "Gray" to match our old defaults. Now it actually behaves like our current highlight: i.e., it follows the opacity set in the menu. (And it's the only one doing that, so the label has been updated accordingly).

[poire-z]

should we keep the fancy colored backgrounds in the color picker on grayscale devices?

I guess why not, if you're showing that menu: it could allow B&W-device-only people to chose another shade of gray for different kind of highlight, still being able to know in what color it will look in their futur device (when we will be all forced to get a color device, when KOReader will only work on color devices, because you will get a color device...).

[hius07]

Am I right that on non-color devices we will get new highlights in yellow by default?

koreader/frontend/apps/reader/modules/readerview.lua

Line 99 in 9d7f70b

saved_color = "yellow",

koreader/frontend/apps/reader/modules/readerhighlight.lua

Lines 1935 to 1943 in 9d7f70b

	local hl_item = {
	datetime = datetime,
	text = cleanupSelectedText(self.selected_text.text),
	pos0 = self.selected_text.pos0,
	pos1 = self.selected_text.pos1,
	pboxes = self.selected_text.pboxes,
	ext = self.selected_text.ext,
	drawer = self.view.highlight.saved_drawer,
	color = self.view.highlight.saved_color,

[NiLuJe]

Am I right that on non-color devices we will get new highlights in yellow by default?

That's indeed something I wanted to look into; not entirely familiar with how all this interacts and what's actually currently happening on master ;).

[ghost]

Restored colorful bgcolor support in the color selection popup, because it looked cool ;p.

(Also, it gives me an excuse to actually use the stuff implemented in base).

How to set this in my KOReader? I have a color PocketBook

Does it work?

[poire-z]

Still in the works, this PR and koreader/koreader-base#1680 are not yet merged.

I guess that with these new Kobo color eInk devices coming up, @NiLuJe will get more pressure and won't resist buying (or being offered) one :)

[ghost]

Can I test this color feature? I have a Color E-Reader.
PocketBook 633 Color.
I will be very glad to help!
If I could test this feature right now during development and send you reports maybe?
I want color highlighting sooooooo much!

[ghost]

I think you need to consider choosing the right colors, because some early E-Readers only support 4096 color by 100 dpi (like mine ereader)...
It must be taken into account.

[ryanwwest]

Will this not be merged until after #11563 is merged? Since that is changing up highlights a bunch as well. Looking forward to both PRs.

[NiLuJe]

That's a fair point (and thanks for the heads up, as I haven't had time to catch up yet), it'll probably depend on whenever I actually get time to finalize reviewing & testing this.

[skylarsoulmate]

When approximately?

[NiLuJe]

Absolutely no ETA, as I'm not in a position to guarantee anything right now.