Create separate renderer classes

[tmssngr]

I've tried to extract the computeSize and paint code into separate *Renderer classes with one abstract parent class, e.g. ScaleRenderer and an implementation class like WindowsScaleRenderer. For buttons, I've also split to separate classes for checkbox, radio button, arrow button and "normal" (AKA push/toggle) button.

Those renderer instances are created by a factory owned by the Display.

[tmssngr]

This will fail ATM on non-Windows systems (because Display was not updated there yet), but I wanted to get your feedback first.

[github-actions]

Test Results

   341 files  ±0     341 suites  ±0   2m 40s ⏱️ +14s
 3 955 tests ±0   3 671 ✅ ±0  284 💤 ±0  0 ❌ ±0 
11 693 runs  ±0  10 808 ✅ ±0  885 💤 ±0  0 ❌ ±0 

Results for commit bf07024. ± Comparison against base commit 6bf05bf.

♻️ This comment has been updated with latest results.

[HeikoKlare]

Some quick feedback:
Separating the renderers from the widgets themselves makes sense to me, so that's definitely a great improvement.
I am not completely sure about the intention with the Windows...Renderer implementations. Is the goal to implement OS-specific renderers for all widgets? For me it would be important that we do not require for every widget that is custom implemented to have a renderer for each of the OSes, as that would mean too much effort for the initial contributions and we really need to focus on having more widgets custom implemented than having each widget implemented in different shaped. May be that we eventually want to have different renderers, but I am also not sure whether they will or should be according to the current OS-specific look&feel or whether they may be "themes" that are independent from that.

[tmssngr]

Thanks for the quick feedback.

Well, the current controls imitate the Windows look, hence I've named them that way. For Label and Link I only have implemented a BasicLabelRenderer and BasicLinkRenderer. Of course it is not required to create all OS implementations, but this PR should illustrate how it could be done if one needs it. I'd rather have the intention to create a platform independent, good look similar to FlatLAF for our applications.

One aspect that this PR discussion should clarify is where to store the (official and internal) state of the controls. The offical state is about those properties available from the control's official API (e.g. button.setGray(true)). The internal state would be something like the mouse-over or pressed state (while clicking a button) that influence rendering. I'm inclined to have all these states be part of the control-specific's root renderer, e.g. ButtonRenderer, while the control (Button) reads and writes it. The implementing renderer (WindowsButtonRenderer) then is a subclass of this control-specific's root renderer. The parent's of the root renderer could follow the same logic as the control's hierarchy (see ControlRenderer) - later also containing the enabled state (I'm not sure about the visible state as this is not drawing related).

[DieterMai]

I don't really like the "Windows" in the name of the renderers. During implementation i try to emulate the rendering of the windows native widgets but this is just to compare with the native implementation. I see this as an intermediate step. As soon as the SWT skija implementation reaches feature parity, the look&feal should get an modern, not-OS-related look. In that case Windows*Renderer would not make sense and for now it might suggests that each renderer would have to look like windows.

[tmssngr]

What prefix you'd prefer instead?

[DieterMai]

What prefix you'd prefer instead?

Either no prefix or Default or, if we decide to give the default design a proper name, use that is prefix. EG. FlatLafButton. This is far the the future though.

[tmssngr]

For the copyright-header: when extracting major parts of one class to another, should I also copy the original copyright-header?

[tmssngr]

Do you still see some changes that I should fix before merging?

[HeikoKlare]

Thank you for all your comments and the discussion. I see there is already a good agreement.

Well, the current controls imitate the Windows look, hence I've named them that way.

I understand, thank you for the clarification! So it was not about providing renderers for each OS but just indicating what the renderer is about (currently, providing Windows look&feel, also on, e.g., Linux). But I think you already found a good agreement and we may change the names of the renderers later on once we agree which renderers we want to have.

One aspect that this PR discussion should clarify is where to store the (official and internal) state of the controls.

Even though the answer might be unsatisfying, I would say that it depends (like you also argued in your further comment): in case it's state about that widget that would, e.g, need to be preserved upon exchanging the renderer, it should be stored in the widget itself (like its enablement, visibility or checked state). If it's about state of the rendering or some transient state that can be restored (like hovering), it might be part of the renderer.

For the copyright-header: when extracting major parts of one class to another, should I also copy the original copyright-header?

I am not sure whether there is any strict policy since the actual contributions are documented via version control anyway. When a class is based on an existing other class, I usually copy over the copyright header from there, as it's basically an evolved version of that file.

Do you still see some changes that I should fix before merging?

To me it looks good now. I am only wondering about the different RendererFactories: since they are all the same right now, shouldn't we defer creating separate factories until we actually have differences in them? Good chance that there won't be Windows/Linux/MacOS factories but rather ones according whatever kinds of renderer "themes" we provide.

[tmssngr]

Even though the answer might be unsatisfying, I would say that it depends (like you also argued in your further comment): in case it's state about that widget that would, e.g, need to be preserved upon exchanging the renderer, it should be stored in the widget itself (like its enablement, visibility or checked state). If it's about state of the rendering or some transient state that can be restored (like hovering), it might be part of the renderer.

Let's look at the Image of a Button or Label: Usually, the image belongs to the control and should be kept when switching the renderer (in the future). However there also exists a cached disabled image which is created lazily when rendering a disabled control. If it should be stored in the renderer, we need to tell the renderer to invalidate it. If it should be stored in the control, it needs to be made accessible by the renderer. I'd opt for storing it in the renderer.

[tmssngr]

For the Button I now have moved the properties back into the control, but for the Label I don't see a good way because, e.g., it does not expose all background related properties.

[DenisUngemach]

1.) Why do you bind the RendererFactory to the Display?

I would rather create a singleton factory, which then returns the right Renderer dependent on the input control:

``
Control buttonWithRadioButtonStyle ...

var renderer = RendererFactory.getInstance().createRenderer(buttonWithRatioButtonStyle); // returns the right renderer for Radiobuttons

``
2.) A renderer should just create the image for an input. In your case the renderer also contains the control attributes. This mixes up the concepts.

I would rather prefer stateless renderers, which gets all control attributes from the control.

Let's look at the Image of a Button or Label: Usually, the image belongs to the control and should be kept when switching the renderer (in the future). However there also exists a cached disabled image which is created lazily when rendering a disabled control. If it should be stored in the renderer, we need to tell the renderer to invalidate it. If it should be stored in the control, it needs to be made accessible by the renderer. I'd opt for storing it in the renderer.

In this design, the disabled image is a cached data for the image. Which means if you change images the renderer should invalidate its cache.
This also means the disabled image is not an attribute of the label, but cached data in the renderer in order to prevent unnecessary recalculations.

If the disabled image is created in the label and the renderer requests it from there, then it is an attribute.
There i also don't see a problem to give the renderer access to this attribute. I think both is possible.

In my opinion all necessary label attributes should be made accessible for the renderer.
For example with package protected attributes.

Or it is possible to create a ControlAttributes class, which then contains all attributes of the control.
Then ButtonAttributes, Label Attributes etc. extend ControlAttributes.

Maybe this should be clarified first.

Nonetheless, I think a renderer design would be useful.

[DieterMai]

Or it is possible to create a ControlAttributes class, which then contains all attributes of the control.
Then ButtonAttributes, Label Attributes etc. extend ControlAttributes.

I thought about something like this before, but backed away from it because i did not want to over engineer things from the start.
But there are several advantages:

• The Renderers would be mostly stateless
• The Renderers would not need a reference to the control since they can get everything they need from the *attributes class which is injected.
• The Renderer could store the last used *Attributes Object/Record. If the next render() is called with the same attributes class, no rendering is required and we save us some operations.

void render(GC gc, ButtonAttributes attributes){
    if (attributes.equeals(cashedAttributes)){
        return;
    }
    cashedAttributes = attributes
    ...
}

But there is also a disadvantage: We don't know in advance what attributes will be required by a custom Renderer. Thus we would have to add all attributes, event those that are currently not used by our Renderer implementation. This could be a performance hit but i image a rather small one.

[tmssngr]

1.) Why do you bind the RendererFactory to the Display?

Because it is the natural central for all GUI related stuff and I would expect to have a method display.setRendererFactory(...) (or display.setColorProvider(...)) that also updates the existing controls (in the future).

I would rather create a singleton factory, which then returns the right Renderer dependent on the input control:

I consider singletons mostly as anti-pattern. Better avoid them if you can - they make discovering a feature harder, they make detecting dependencies harder, they make testing harder.

2.) A renderer should just create the image for an input. In your case the renderer also contains the control attributes. This mixes > up the concepts.

I would rather prefer stateless renderers, which gets all control attributes from the control.

There are two kind of states, the official API states like text, selected, grayed, and the internal states like mouseOver, pressed. IMHO the latter rather belong to the renderer. Also you don't need to forget the API of the controls: should they really open their internal state to the public? Better let the control set some state on the renderer.

In my opinion all necessary label attributes should be made accessible for the renderer.
For example with package protected attributes.

It should be possible to implement the renderer in any package. This would not be possible with making it (package) protected.

[tmssngr]

* The Renderers would not need a reference to the control since they can get everything they need from the *attributes class which is injected.

Currently they need a reference not just to access states (isFocus(), getSelection(), ...), but also for rendering (to get the Drawable).

* The Renderer could store the last used *Attributes Object/Record. If the next render() is called with the same attributes class, no rendering is required and we save us some operations.

This best would be done with immutable states (records).

[tmssngr]

Currently, #154 causes me to be reluctant in merging (AKA splitting the Link code into multiple files) before this bug is fixed.

[DenisUngemach]

1.) Why do you bind the RendererFactory to the Display?

Because it is the natural central for all GUI related stuff and I would expect to have a method display.setRendererFactory(...) (or display.setColorProvider(...) that also updates the existing controls (in the future).

There can be multiple Display instances, so if someone changes the RendererFactory in one Display, multiple RendererFactories with different configuration could exist.

I would rather create a singleton factory, which then returns the right Renderer dependent on the input control:

I consider singletons as anti-features. Better avoid them if you can - they make discovering a feature harder, they make detecting dependencies harder, they make testing harder.

Singleton Pattern: I don't see it that way, especially because of the alternatives. But this is a general discussion.

2.) A renderer should just create the image for an input. In your case the renderer also contains the control attributes. This mixes > up the concepts.
I would rather prefer stateless renderers, which gets all control attributes from the control.

There are two kind of states, the official API states like text, selected, grayed, and the internal states like mouseOver, pressed. IMHO the latter rather belong to the renderer. Also you don't need to forget the API of the controls: should they really open their internal state to the public? Better let the control set some state on the renderer.

In my opinion all necessary label attributes should be made accessible for the renderer.
For example with package protected attributes.

It should be possible to implement the renderer in any package. This would not be possible with making it (package) protected.

When i think about animations, then it might make sense.
Then the renderer should draw the animation with precise animation states. And the Button should not know these specific animation positions.

This would be the idea of custom animations by custom renderers for a control. This is thinkable...

But this argumentation breaks if the rest of the control also needs a specific information.

Currently i am not sure about this.

@HeikoKlare
What do you think?

[tmssngr]

Out of curiosity: what is the idea behind having multiple display instances?

Heiko hat a similar suggestion in my color provider PR #130, but I could convince him;).

[DenisUngemach]

Multiple Display instances is a Windows feature.
For each Display instance you have a "main thread".

This means you can run one java program, and have two shells which are thread independent and don't interfere with each other.

[HeikoKlare]

Multiple Display instances is a Windows feature.
For each Display instance you have a "main thread".

Common use case for this is asynchronous "off-screen" rendering. We use that to keep the UI responsive while doing a complex rendering on a separate display and copy over the result once that's done.

[tmssngr]

Thanks for the usecases - I've did not know them. Could it be useful to set one display a different color-provider/renderer-factory than the other?

[HeikoKlare]

Could it be useful to set one display a different color-provider/renderer-factory than the other?

I dont't think so.

[tmssngr]

Does someone object to be merged in the current state?

[HeikoKlare]

I am fine with the changes. I have just tested them and did not find any introduced issues so far.

I only saw an accidentally removed copyright header. Once that is readded, I am fine with merging.

[tmssngr]

I've extracted the LinkRenderer stuff into a different branch because it is not yet ready (the properties are not moved back yet to the control, because the Link needs to be reworked anyway #154).

[fedejeanne]

Why does it have to be a CustomControl instead of a Control? I was trying to adapt my current PR (#150) to use the renderers but since Spinner still extends Control then I get a compilation error.

[fedejeanne]

btw I tried simply changing the type to Control and adapting 2 other methods to accept Control as parameters and everything compiles and seems to run fine (I tested the ControlsExample). I just ask in case this was not a mistake and I am missing the big picture :-)

[tmssngr]

As Spinner is a Composite that can't be a child of CustomControl, your change is fine.

[tmssngr]

Maybe it could make sense to make ControlRenderer generic: ControlRenderer<C extends Control> and making the field control protected, so subclasses can easily use it?