consistent model color scheme

[pett]

Discussion from email thread:

Yes, we need to come up with something mnemonic and consistent. For now, I am just having the Model Panel issue the appropriate 'color' command, though for those that have to resort to the 'models' target the undo won't be correct until this gets sorted out.

--Eric

On Apr 15, 2021, at 6:55 PM, Tom Goddard <goddard@…> wrote:

If git says I wrote that I guess we have to believe it. It saves the single color and vertex colors and the auto-recoloring state for a Model. Keep in mind that color command _set_model_color() really appears to be "_set_nonmolecular_surface_color()" -- that is the only case it is called. I probably spent all of 15 minutes designing that code. So I' would not jump to apply it to everything without careful thought.

I am a little worried that Model.color and Model.model_color is going to cause a lot of confusion. Maybe it can be remedied by adequate doc strings. Currently Structure, Volume and VolumeSurface all override Drawing.set_color() which is used by property Drawing.color. That stuff was all done before single_color existed and I think those overrides were trying to do the same thing as single_color -- ie color model subdrawings, not just set the Drawing._color attribute which would not even be visible. So I am unclear how this mess is going to be sorted out. I think the lower level graphics module Drawing needs to keep its simple notion of having a single color (property "color") and overriding that was probably not a good idea. Or maybe it is a good idea and there should be no "model_color". Maybe a better design would be that Drawing.single_color is the low level color for the drawing, not to be overridden, and Model.color by default uses single_color but is intended to be overridden and might color subdrawings, and also might compute some average color from subdrawings.

Let me know what you come up with.

Tom

On Apr 15, 2021, at 6:12 PM, Eric Pettersen <pett@…> wrote:

Hi Tom,

When you were speculating about how to undo single-coloring of models, apparently you'd forgotten that you'd already implemented a scheme for it. The Model.color_undo_state property is used to get/set the coloring of a model. At least, that's what _set_model_colors() in std_commands/color.py (written by you) is using it for. Seems like we should just keep using that (and probably implement it for more models -- like Structures!).

--Eric

[pett]

Well, my leaning here is to change Model.model_color to something else and document the hell out of the difference between it and Model.color (leaving Model.model_color around as an undocumented synonym for the new name for backwards compatibility). My reasoning is the Model.color always returns an actual color whereas the used-to-be-model_color sometimes returns None. So the question is what to call it. My choices in rough order of preference are:

overall_color
main_color
primary_color
dominant_color
most_common_color

I think 'single_color' would still be too confusing with Model.color.

On the 'undo' front, I'm reasonably happy with your Model.color_undo_state scheme and would expand its use to cover Structures and perhaps other model types.

[Tom Goddard]

I have never figured out what the semantic difference is between these two color attributes. The overall_color seems to be the Model Panel color. If it is only the model panel color maybe best to name it model_panel_color. But maybe it is more than that. Setting this overall_color can set the color of child drawings if the parent so desires. Does code other than model panel do that? The other "color" is Drawing.color, and it does not set anything except the one drawing's color for rendering graphics. Some code has overridden this property like VolumeSurface to also change the clip cap color. That override seems like a bad idea, confusing. Probably Drawing.color should never be overridden and if setting the color needs to do fancy things that should be a new attribute like overall_color. When would the color command set "overall_color" versus setting "color"? Which would it set for command "color #3 blue target s" if #3 is some derived class from Surface?

Of your names I like overall_color the best since it conveys best that this color might effect subdrawings (e.g Structure.overall_color effects atoms and ribbons).

[pett]

model_color is the generic way of getting/setting model colors and is most useful in user interfaces. The model panel uses it this way, but the sequence viewer also uses it to determine the color swatch to put behind the sequence name when there are associated structures.

I think the problem that kicked this off is that the model panel needs to generate a command when a color button is clicked (it used to directly set Model.model_color), and "color #N <color> target m" seems like it would be ideal (it sets model_color), but currently "target m" is only undoable for surfaces, not structures. Therefore right now the model panel generates different color commands depending on what the model type is.

Based on your last comments, here's a modified proposal:

1) Change model_color to overall_color
2) Change overrides of Model.color to override Model.overall_color

a) Change germane settings of Model.color to Model.overall_color

3) Implement Model.color_undo_state for Structures and possibly other model types

Which would it set for command "color #3 blue target s" if #3 is some derived class from Surface?

It would do the same thing it does now I think, which is set overall_color for non-molecular surfaces and execute a bunch of special-purpose code for molecular surfaces (to handle byhet and whatnot).

[Tom Goddard]

Seems reasonable.

[pett]

Basically just changed uses of Model.model_color to Model.overall_color, which is a mild improvement. Didn't revamp model panel code or color command / Structure code to use simplified code that would be possible by implementing Structure.undo_color_state, since the current code works fine and is a sunk cost. If other problems crop up where implementing undo_color_state would help, then might invest the time to do it.