[Cuis-dev] Theme>>useUniformColors considered harmful

[Casey Ransberger]

I’m poking around in the vicinity of Theme, thinking about adding a class to Theme-Core. When I initially wrote Theme, I did a lot of gymnastics to keep it down to a single class, wanting to avoid unnecessarily adding a bunch of classes to the system. Also wanted to impress Juan, who I expected would be hesitant to add a bunch of complicated machinery to Cuis.

Looking back on the code something like a decade later, I think I was a little gung-ho with the one-class thing. Adding a single class UniformColorTheme (or maybe we can think of a better name like BoringColorTheme) seems to obviate #useUniformColors completely, and thus eliminate 13 send sites (which, I might add, all have a code duplication smell.) Each of these send sites involves branch logic that would go away, 3 or 4 lines of code each. I haven’t investigated this last one yet, but #defaultWindowColor might also go away, yielding a bit more in the way of complexity savings.

Here’s a typical send site of Theme>>useUniformColors.

Theme>>versionsBrowser
	^ self useUniformColors
		ifTrue: [ self defaultWindowColor ]
		ifFalse: [ `(Color r: 0.869 g: 0.753 b: 1.0) duller` ]

I’m not sure if I wrote the above code or not.

It’s always ^self useUniformColors ifTrue:[] ifFalse[]. All 13 sites.

Between one and two methods would go away along with an additional 39+ lines of code. In exchange, we get a new class (my vote is for BoringTheme) with as many as 13 one-line methods. A cross-cutting concern goes away. We lose 5 LoC to a class definition, if we want to even count that, still a 21+ line win. I haven’t looked at the extra themes, but I’ll update them if I make the change, and I expect code savings there as well. More importantly, it will be more logical, more object oriented, less tricky. Right now to implement a theme, you need to know about this odd convention in these 13 methods, and it shouldn’t be that way. I went with methods instead of sticking instances of Color in ivars so we could experiment with functional expressions that yield arbitrary UI objects (originally there was going to be a lot more to this than colors, but life has a way of interrupting grand schemes.) Here though we’re throwing in logic to pick between two colors, which smells like the opposite idea.

Okay that’s the gist of it. Only performance impact should be negligible but positive (sends and branch logic that don’t need to happen anymore.) Theme-Core isn’t at all a crowded category so scrolling and cognitive load shouldn’t be an issue with adding a class here. I can probably absorb what I think used to be called GrayTheme out of the extra themes package into OfficeDull Win95KnockOff or whatever we decide to call it, so any methods there go away from the extra themes package.

Is everyone okay with me doing this?

—Casey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20200525/e531d0c6/attachment.htm>