Highlights: improve 'Set color' dialog#14699
Conversation
| shrink_unneeded_width = false, -- have 'width' meaning 'max_width' | ||
| shrink_min_width = nil, -- default to ButtonTable's default | ||
| tap_close_callback = nil, | ||
| colorful = false, -- should be set to true if any of the buttons' text is colorful |
There was a problem hiding this comment.
Colourful usually means, that something has a lot of colour and is vibrant (is not the opposite of monochromatic). I would suggest you find a different name as that is not what seems to be the case here.
There was a problem hiding this comment.
Wouldn't a plain color be enough?
There was a problem hiding this comment.
I think there was a call point with color in there already (at least at some point), which explains the roundabout naming. (I was never really happy with it, though, so if you find something better, go ahead).
Basically, the idea was to not confuse that with setting an actual color (which is what our color variables usually mean), hence colored/colorful ;).
|
@NiLuJe and possibly @hius07 too. Sorry, not sure how to message you guys otherwise, but I've been thinking about making a discord server for KOReader for a while, and as I think you guys maybe are some of the main contributors or possibly are in charge of this repo, I'd like to talk to you about it. You can reply here, or I'm veevui on discord or veeevui on reddit. |
| function ReaderHighlight:getHighlightColorHash(color) | ||
| return Blitbuffer.colorFromName(color) --- @todo fix in the night mode; issue #14170, #14667 | ||
| or Blitbuffer.gray(self.view.highlight.lighten_factor) -- 'gray' or unknown color | ||
| end |
There was a problem hiding this comment.
It's possible I see too much of the word "hash" in crengine and at work, and it evokes for me https://en.wikipedia.org/wiki/Hash_function (and not being young, it doesn't evoke "hash tag koreader" at all :)).
I think here, it returns the hex value (#BB4499) of that color ?
https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/color_value calls it "hex color".
So, maybe getHighlightColorAsHexColor or getHighlightHexColor ?
There was a problem hiding this comment.
If it means hex I didn't realize from the name either. ;-)
There was a problem hiding this comment.
It returns ColorRGB32 or Color8 structure.
Isn't it a "hash table"?
There was a problem hiding this comment.
Oh, yes, a hash table ala Lua (ie. not an array-like table).
But here, that hash table would not really be just a hash table, but more like an object, an instance of these struct, which are somehow subclasses of the generic class Color (even if it doesn't exist, haven't checked).
So, then, it just returns a "color", what we use everywhere we set color = (ie. Blitbuffer.COLOR_DARK_GRAY).
So, just getHighlightColor(color_name) ?
|
@Frenzie, koreader/koreader-base#2233 is waiting for you. NiLuJe has approved. |
|
@hius07 Are you just asking if I also think it's good or do you mean I didn't give you the necessary access rights? |
|
In any case your "okay" is desirable. |
|
Fair enough. |
Introducing colored buttons.
This change is