-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add color highlight menu #11044
Add color highlight menu #11044
Conversation
Initial inspiration. #9024 (comment) |
Since the color is constant within a page, it can be calculated outside of the loop on each box. |
In view of this in the future, the general setting should be done as a submenu, not RadioButtonWidget, similar to highlight style. |
BTW, we do not have a feature "apply default style to all highlights in the book", it might be useful. |
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. |
Newest revision now supports per-highlight colors. This extra info should be part of highlight exports, thus making it queryable to outside integrations. @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. @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. |
Latest revision allows for customizable colors in settings.lua
Edit color button is now completely removed on non-color devices. |
Newest revision should now pass in pdf annotation colors to mupdf backend. |
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. |
Custom colors are now with patches. |
Should we really save the rgbcolor ( |
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). |
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 |
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.. |
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. |
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... |
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. |
ReaderHighlight now stores string names, and ReaderView now has a name->rgb lookup table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 8 files at r19, 1 of 1 files at r20, 1 of 1 files at r21, all commit messages.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @hius07, @poire-z, and @smasher816)
ChangeLog:
(Depends on koreader/koreader-base#1680) |
Commit history will be archived in my color-highlights branch |
return T(_("Highlight color: %1"), string.lower(v[1])) | ||
end | ||
end | ||
return T(_("Highlight color: %1"), saved_color) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what color?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the question ;).
But: all of them? This is to set the defaults, so you have access to all of them (including gray).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is executed when saved_color
is not in self.highlight_colors
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand where you're going with that, sorry ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the color is unsupported (somehow, that would require some pretty heinous trickery I guess?), this would show the actual color name stored, but it would be rendered as gray.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, how did the unsupported color appear in
saved_color
?
It shouldn't, unless someone manually mangled annotations, or we somehow fuck up the list of color names in the future ;).
I'm not reading all the context of this pull request, but I'd appreciate if all possible RBX hex colors (not just the ~10 or so official ones we allow users to pick) can still be displayed in their proper color, instead of grey.
They technically can, but that requires you to user-patch the list of name = code in blitbuffer for what you want to use.
(i.e., annotations store a color name, not a color code; and it's up to the device to render that according to what it thinks that color should be mapped to. I guess the intent was to be able to move annotations between devices where some of them can be very very bad at rendering certain colors).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't, unless someone manually mangled annotations, or we somehow fuck up the list of color names in the future ;).
Oh, or, if, I guess, someone user-patched both blitbuffer and readerhighlight to add more color names, and that was moved to another device where those color names are unknown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annotations store a color name, not a color hash
Hmm, okay. I understand the idea of different hex codes for the same color depending on the device (but also wonder if that would ever actually happen).
My eventual idea is syncing these annotations between KOReader and Zotero and since Zotero stores hex codes, not color names (and allows any hex combination), I imagined this would be similar. But, that's just hypothetical and may never happen so doesn't make sense to design it here just for that. Still, that's my reasoning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the original discussion you were part of last year at the top of this issue, I didn't come up with that, I just picked up the baton ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already coded like this, and I bet a plugin could somehow make it work with this code. Like e.g. from what you said
[a plugin] user-patched both blitbuffer and readerhighlight to add more color names, and that was moved to another device where those color names are unknown
So it's fine. Zotero doesn't have underlining/invert highlight style anyway so those I'd need to some sort of mapping in KOReader regardless.
Don't do anything if the device doesn't support Kaleido wfm, or if color is disabled. Also, use full to prevent partial being "upgraded" to flashui, which wouldn't be eligible for Kaleido (on Kobo, at least, PB has a less granular wfm selection, so it doesn't care about this distinction).
annotation expert @hius07 ;).
if Blitbuffer.isColor8(color) then | ||
bb:paintRect(x, y + h - 1, w, Size.line.medium, color) | ||
else | ||
bb:paintRectRGB32(x, y + h - 1, w, Size.line.medium, color) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You draw the note marking underline with the same color as the highlighting itself, not visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine we probably actually want to keep those in black, always, not only the underline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although... I can sort of see the sideline being in color somewhat useful?
Mildly less enthused about the icon, but I'm really, really not the target audience, so, no idea ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, the icon is already B&W only, so we're good ;p.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Underline and icon always in black I think, sideline on b/w devices in black and on color devices maybe in color.
indeed doesn't make any sense, good catch ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok. Trusting @hius07 for all the widgets in ReaderHighlight.
Looks like we're showing the Highlight color menu/widget item even on B&W devices, right?
In #11044 (review) and next one, @hius07 wanted them only on color devices. Is that resolved?
Yep, it allows modifying colors on imported highlights. (Or, hell, even on the same device if you happen to disable color rendering on it ;p). I think the original comment was from when the color button was added to the initial on-hold strip of buttons widget thingy; this is no longer the case, it's just an extra row at the bottom of the Style popup now. |
Imported highlights between a color device and another b&w device must be a rare use case. |
Yep! |
This is awesome, thanks so much to everyone for making this happen!! |
Many thanks to @smasher816 for the initial implementation, and to @hius07 for his help pushing this through the finish line during the various iterations of this PR ;). |
Does this mean the feature is going to be in the next build/ release? Awesome, many thanks guys. |
You are making the best day in a girl’s life a reality. How wholesome. |
Looks like I get a crash when I open a book starting on a page with a highlight (made before this PR I guess) - and also after opening on a blank page when going to a page with a highlight:
Is it just me? |
It happened to me also. My workaround was to apply the default highlight style to all from the menu. |
Went with: - local color = Blitbuffer.colorFromName(item.color)
+ local color = Blitbuffer.colorFromName(item.color or "blah") to move on. By killing colors early, we act for a better future #11044 (comment) |
(They'll be drawn them in gray, as before) Regression since koreader#11044 (koreader#11044 (comment))
#12438) (They'll be drawn in gray, as before) Regression since #11044 (#11044 (comment))
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
Fix #9024
ChangeLog:
colorful
field), and use that information to make sure the refresh will use a Kaleido waveform mode on compatible devices.(Depends on koreader/koreader-base#1680)
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)