Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Style struct #55

Merged
merged 6 commits into from Jan 20, 2021
Merged

Add Style struct #55

merged 6 commits into from Jan 20, 2021

Conversation

mrichards42
Copy link
Collaborator

@mrichards42 mrichards42 commented Jan 20, 2021

Based on #50 and #51 this adds a Style struct to all widgets.

I've left in the concept of defaults, but instead of those being individual static members of the style class (DEFAULT_FS, DEFAULT_JUSTIFY), there's now a default Style, so anything in the Style struct can have a default.

The style system involves three classes:

  • Style -- a simple struct holding all style information.
  • Stylesheet -- used to update individual parts of a Style.
  • Stylesheet::Inherited -- a Stylesheet used to copy some variables from one Style to another.

The approach this PR suggests to managing styles is to create a Stylesheet and apply it to widgets as appropriate. Some widgets have default stylesheets (e.g. Button). For instance, suppose you have two different kinds of buttons you might want to support in your app. One with an outline and centered text, and the other with an underline and left-aligned text. This might look like:

// Although note valign and border are not implemented yet.
Stylesheet OUTLINED_BUTTON_STYLE = Stylesheet().border(1).valign_middle().justify_center();
Stylesheet UNDERLINED_BUTTON_STYLE = Stylesheet().underline(true).valign_top().justify_left();

auto btnA = new ui::Button(...);
btnA->set_style(OUTLINED_BUTTON_STYLE);

auto btnB = new ui::Button(...);
btnB->set_style(UNDERLINED_BUTTON_STYLE);

Alternate approaches

I started going down the road of creating a separate style hierarchy that would be used via multiple inheritance. That's a little more flexible, but less straightforward and IMO a little more confusing (as things always are w/ multiple inheritance). The advantage is mostly that you don't have to store every style in every class, since not all styles will be relevant in every class. There's probably a way to get something similar using has-a instead of is-a, but the solution wasn't immediately obvious to me. An example:

struct StyleBase {
  virtual ~StyleBase() {} // needs to be polymorphic for Stylesheet to work
};

struct TextStyle : StyleBase {
  int font_size;
  bool underline;
}

struct AlignStyle : StyleBase {
  enum JUSTIFY { ... }
  enum VALIGN { ... }
  JUSTIFY justify;
  VALIGN valign;
}

class Text : public Widget, public TextStyle, public AlignStyle {
  ...
}

auto text = new ui::Text(...);
Stylesheet().font_size(32).justify_center().apply(text);

BREAKING CHANGE: existing apps that mutate member variables directly
will need to be edited to either use ui::Stylesheet (preferred), or at
the very least, mutate those variables in the `style` struct.

Example, setting font_size:

    // The old way:
    myWidget->font_size = 32

    // The new way (encouraged):
    const ui::Stylesheet MY_WIDGET_STYLE = ui::Stylesheet().font_size(32)
    myWidget->set_style(MY_WIDGET_STYLE)

    // The new way (discouraged, but still possible):
    myWidget->style.font_size = 32

Example, setting style defaults:

    // The old way
    ui::Text::DEFAULT_FS = 32
    ui::Text::DEFAULT_JUSTIFY = ui::Text::JUSTIFY::CENTER

    // The new way
    ui::Style::DEFAULT.font_size = 32
    ui::Style::DEFAULT.justify_center()
    ui::Style::DEFAULT.underline = true   // not supported before now
src/common.make Outdated Show resolved Hide resolved
src/rmkit/ui/button.cpy Show resolved Hide resolved
@@ -65,13 +69,6 @@ namespace ui:
input::SynMotionEvent fake
self.on_mouse_click(fake)

// function: set_justification
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced by set_style()

Copy link
Member

@raisjn raisjn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is super cool, code looks great - i'll compile and test things out

src/common.make Outdated Show resolved Hide resolved
src/rmkit/ui/button.cpy Show resolved Hide resolved
Comment on lines +12 to +13
// When adding a new style, make sure to also add builders in Stylesheet
// and Stylesheet::Inherited.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if only there was easier metaprogramming :-(

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, there's always macros :)

@raisjn
Copy link
Member

raisjn commented Jan 20, 2021

most things compile (remux, harmony, mines, sharenote, simple, etc) except for minesweeper:

diff --git a/src/minesweeper/main.cpy b/src/minesweeper/main.cpy
index d31a93b..f7855e4 100644
--- a/src/minesweeper/main.cpy
+++ b/src/minesweeper/main.cpy
@@ -37,7 +37,7 @@ void main_menu()
 void resize_field(int)
 void show_scores()
 
-ui::Style LARGE_TEXT = Stylesheet().font_size(64)
+ui::Style LARGE_TEXT = ui::Stylesheet().font_size(64).build()
 
 class SizeButton: public ui::Button:
   public:
@@ -391,7 +391,7 @@ class App:
     button_height := 200
     for auto p : sizes:
       btn := new SizeButton(w/2-400, 500, 800, button_height, p.first, p.second)
-      btn->y_padding = (button_height - btn.style.font_size) / 2
+      btn->y_padding = (button_height - btn->style.font_size) / 2
       size_button_container.pack_start(btn)
       debug "BTN", btn->x, btn->y
 
@@ -401,7 +401,7 @@ class App:
 //    size_button_container.pack_start(new SizeButton(w/2-400, 50, 800, 200, 20, "20x20"))
 
     scores := new ScoresButton(w/2-400, 500, 800, button_height)
-    scores->y_padding = (button_height - btn.style.font_size) / 2
+    scores->y_padding = (button_height - scores->style.font_size) / 2
     debug scores->x, scores->y
     size_button_container.pack_start(scores)
     debug scores->x, scores->y

but this made me think that you maybe had plans for valign (which would make these lines obsolete).

i also tested out remux, harmony, mines, simple - everything looks good so far

@mrichards42
Copy link
Collaborator Author

Woops -- sorry I was doing test compiles along the way, but that diff was a last-minute change and I must not have tried to compile it.

Yep, I was thinking about having valign be the next PR.

i also tested out remux, harmony, mines, simple - everything looks good so far

Awesome, thanks for testing those. I think I made changes in the right places, but I don't know enough about the rest of the apps to know if something was broken visually.

@mrichards42
Copy link
Collaborator Author

Latest commits should address c++17 syntax, the failed minesweeper build, and operator+=

LMK if you'd prefer rebasing those into the previous commits instead of adding new ones.

@@ -241,3 +242,7 @@ namespace ui:
void switch_mode(int mode):
// TODO
pass

;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were a couple files where it looked like okp wasn't adding the semicolon after the last class. Maybe when there are multiple classes in the file?

return

void set_style(const Stylesheet & style):
Widget::set_style(style)
self.textWidget->set_style(Stylesheet::Inherited(self.style).text_style())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

      self.textWidget->set_style(Stylesheet::Inherited(self.style).text_style())

vs

      self.textWidget->set_style(self.style.inherit().text_style())

@raisjn raisjn merged commit cf7d1f4 into rmkit-dev:master Jan 20, 2021
@raisjn raisjn mentioned this pull request Jan 20, 2021
@raisjn
Copy link
Member

raisjn commented Jan 20, 2021

thank you for the diff!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants