Allow alternate colors for the pens #8

asciipip · 2021-04-22T13:50:16Z

This adds render() and command line parameters to specify the colors that should be used for the "black" and "white" pen colors. These can be used to, for example, have annotations in red or blue lines to distinguish from the content being annotated.

The default colors are, of course, actual black and white. The gray pen is always a 50/50 mix of the black and white pens in the RGB color space.

This adds `render()` and command line parameters to specify the colors that should be used for the "black" and "white" pen colors. These can be used to, for example, have annotations in red or blue lines to distinguish from the content being annotated. The default colors are, of course, actual black and white. The gray pen is always a 50/50 mix of the black and white pens in the RGB color space.

rschroll

Thanks for your work on this, and sorry it's taken me a while to get back to this. (Life, you know.)

I made a couple of nit-picky comments below. I'd be happy to merge this in once those are addressed. (And if you don't have time right now, I completely understand!)

But if you want to go further, here's a couple of thoughts:

First, we should also let the user specify the grey color (with a default being set from white and black if it isn't provided) and the highlight color. This shouldn't be too much of a change from the existing work, especially if we just pass a single data structure with all the color values.

Second, I wonder if there's a color name parsing library (for CSS, for example) that we could use here, to let people do --black red instead of --black #ff0000.

Third, some of the other software out there tries to read pen colors from layer names. I probably don't care enough to implement that, but if you think it would be fun, I'd be happy to merge it.

Fourth, I wonder if PIL has an "average colors" function that works in a perceptual colorspace instead of RGB.

You're welcome to work on any of those ideas, either here or in an additional PR. Or none of them is fine too!

rschroll · 2021-05-31T04:38:15Z

rmrl/__main__.py

    parser.add_argument('--no-expand', action='store_true', help="Don't expand pages to margins on device.")
    parser.add_argument('--only-annotated', action='store_true', help="Only render pages with annotations.")
+    parser.add_argument('--black', default='#000000', help="Color for \"black\" pen, in hex notation (#RRGGBB).")
+    parser.add_argument('--white', default='#FFFFFF', help="Color for \"white\" pen, in hex notation (#RRGGBB).")


Nit: Use single quotes on the help string to avoid the need to escape double quote characters.

rschroll · 2021-05-31T04:40:02Z

rmrl/__main__.py

    args = parser.parse_args()

+    for color, value in [('White', args.white), ('Black', args.black)]:
+        if re.search(r'^#[0-9a-f]{6}$', value, re.I) is None:


Instead of looking at a form, let's just try parsing the input into a color and aborting if that fails. That way, we can improve the color parsing code without need to update this code as well.

Sure. The new code just passes the strings to render() as-is. render() does the parsing and raises an InvalidColor exception if the parsing fails.

rschroll · 2021-05-31T04:45:02Z

rmrl/__main__.py

-                    only_annotated=args.only_annotated)
+                    only_annotated=args.only_annotated,
+                    black=parse_color(args.black),
+                    white=parse_color(args.white))


In anticipation of making the grey and highlighter colors configurable, I'm thinking we should pass a color dictionary through all of the layers. (Or maybe a namedtuple? Or dataclass?)

Hm. I think a namedtuple probably makes the most sense, with keys for black, white, grey, and highlight.

asciipip · 2021-06-03T21:24:02Z

You're probably right about being able to specify all of the pen colors (black, white, grey, and highlight). Do you think it makes more sense to have separate command line options for each (--highlight #FFCCCC, --grey #0000AA, etc.) or a more generic (but more complicated to parse) --color option (e.g. --color highlight=#FFCCCC --color grey=#0000AA)?

I'll look around to see if there's a useful library for parsing colors that includes names.

Colors from named layers might be interesting, but I'd view that as a separate thing from what I'm doing here. Maybe that'll be something I try to tackle at a later point. (Personally, I'm hoping the reMarkable developers add some sort of support for setting pen colors at some point, either per-layer or per-notebook.)

For the color averaging, I've used the colormath library for such things in the past, but it seemed like overkill here. (I think that averaging the channels in RGB space is good enough for most cases, and people will be able to override that default if they want after I flesh out the color setting options.)

asciipip · 2021-06-03T21:50:39Z

Okay, it looks like there are a few options for color parsing:

matplotlib - Crazy number of input formats, including XKCD colors. Drawback: matplotlib is kind of a heavy pull for just this use.
colour - A few different input formats, plus it parses into an object that seems useful to work with. Drawback: hasn't been updated in nearly four years.
webcolors - Aims to parse anything that would be a valid CSS color. Seems to be actively updated. Drawback: No single point of entry for the parsing; needs glue code to determine the color representation so you can call the right parsing function.

I'm leaning toward the last one, webcolors, unless you have a strong preference otherwise.

rschroll · 2021-06-04T07:19:09Z

I think we should have four different flags, rather than a generic color option. It's not like this is going to expand to dozens of colors. Also, for integration with rmfuse, I'm going to suggest that the render function takes four options, one per color, and then does the parsing and unification into a single data structure to be passed down the stack. The render function can through a parse error for invalid colors, and the command line version can catch this and display an error message.

If we allow a grey input, then the user can do averaging in whatever colorspace they want, so RGB averaging sounds good to me!

matplotlib is definitely overkill for this. Of the two others, my initial reaction is that the colours API looks nicer than that of webcolors. I'm not that worried about the lack of updates for this sort of library, since color parsing can be solved completely and doesn't change (much) over time. But I'd be happy with either one.

asciipip · 2021-06-08T15:01:18Z

I went with the colour library for this and put in the other changes. The CLI now has four different flags for the pen colors (plus a couple variant spellings). The color strings (or None, which is an option for the gray pen) are passed to render(), which does the parsing. That gives an API that works more or less just like the CLI.

The colour library has a color averaging function of sorts that appears to work in HSL space. I set thing up so rmrl will use that (HSL) averaging if the "black" and "white" pens are both colored and RGB averaging if one or both pens is grayscale. That seems to give good results for me.

rschroll

This is looking great! Thanks for all the careful work.

A couple of comments, but the only one I really care about is renaming the Colors tuple, to make it more distinct from Color.

rschroll · 2021-06-12T07:06:59Z

rmrl/render.py


 log = logging.getLogger(__name__)

+Colors = namedtuple('Colors', ['black', 'white', 'gray', 'highlight'])


Let's call this PenColors, just to avoid confusion with the Color class.

rschroll · 2021-06-12T07:11:37Z

rmrl/render.py

+    try:
+        return Color(color_string)
+    except Exception as e:
+        raise InvalidColor('"{}" color was passed an invalid string: {}'.format(name, str(e)))


Perhaps raise ... from None here?

rschroll · 2021-06-12T07:14:24Z

rmrl/document.py

+            colors.black.rgb,
+            colors.gray.rgb,
+            colors.white.rgb,
+            colors.highlight.rgb,


Namedtuples allow position-based indexing, so as long as we get it in the right order, we could just pass self.colors = colors here, and then do colors[i].rgb in the pen itself.

I'm not completely sold on this idea. I does decrease the amount of fussing needed here, but it also locks our color structure to the details of the lines files from remarkable. I could go either way here, so I'll let you make the decision.

rschroll requested changes May 31, 2021

View reviewed changes

asciipip added 7 commits June 8, 2021 09:19

Use single quotes to avoid escaping internal double quotes

49cd6c3

Use the colour library for color management

e1d1d32

Use a namedtuple to carry the colors around internally

6dfd576

Error handling for the pen colors

28e3de8

Add gray parameter

7333eaf

Add parameter for highlighter color

bd4c52d

CLI docs about the color formats

f523395

asciipip added 2 commits June 8, 2021 11:02

Don't need in __main__ anymore

82d3271

Don't need Color in __main__ anymore

e55e72f

rschroll reviewed Jun 12, 2021

View reviewed changes


		log = logging.getLogger(__name__)

		Colors = namedtuple('Colors', ['black', 'white', 'gray', 'highlight'])

Allow alternate colors for the pens #8

Are you sure you want to change the base?

Allow alternate colors for the pens #8

Uh oh!

Conversation

asciipip commented Apr 22, 2021

Uh oh!

rschroll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asciipip commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asciipip commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rschroll commented Jun 4, 2021

Uh oh!

asciipip commented Jun 8, 2021

Uh oh!

rschroll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

asciipip commented Jun 3, 2021 •

edited

Loading

asciipip commented Jun 3, 2021 •

edited

Loading