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

Fix widgets to behave like real elements #1475

Closed
wants to merge 2 commits into from
Closed

Fix widgets to behave like real elements #1475

wants to merge 2 commits into from

Conversation

jasonrclark
Copy link
Member

Fixes #641

This PR changes Widget to behave essentially as a Flow so it gets all the standard dimensions and styling it wasn't obeying before.

This has a big challenge, though, as the contract for Widget derived classes in older Shoes didn't for calling super from #initialize. We must run UIElement#initialize (or an equivalent) to get everything wired up, but we also need to run the user-defined initialize so the user's code runs.

Ruby's flexibility to the rescue! ✨ We can take the odd step of actually skipping new calls for the widgets and just allocate and initialize (in the order and timing we want, along with our base initialization) directly. 😱 Never thought I'd find a use for that bit of trivia, but here we are!

Other tidbits:

  • Widget delegation before was touchy because it tried to delegate EVERYTHING to the app except a small set of methods we'd called out. Turns out, that's a bad plan, so instead now we delegate everything that the widget class doesn't define itself already.
  • Added a new way for DSL classes to declare their own backend class directly for cases where the class names don't necessarily line up (ala people's custom-named classes, which need to be backed by Shoes::Swt::Flow)

Been using this app, in addition to the two samples with widgets (samples/simple_menu.rb and samples/simple_guess_game.rb):

class MyWidget < Shoes::Widget
  def initialize(color, args)
    background color
    para "thing"
    para "one"
    para "thing"
    para "one"
  end
end

Shoes.app do
  @w = my_widget green, width: 170, height: 120
  click do
    @w.left += 10
    @w.top += 10
  end
end

Copy link
Member

@PragTob PragTob left a comment

Choose a reason for hiding this comment

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

Mainly not so sure I really wanna go with initialize and allocate :)

# turn (via handle_block above) will do the widget class's initialize.
widget_instance = klass.allocate
widget_instance.original_args = args
widget_instance.actual_initialize(@__app__, @__app__.current_slot, container_args)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I'm too happy with this magic of calling initialize/allocate ourselves - how do you feel about breaking compatibility and calling this method initialize_widget instead or something?

Copy link
Member

Choose a reason for hiding this comment

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

Or just require calling super... one of the 2. I dunno, seems like a little breaking change but one worth making imo - don't wanna dabble with initialize and allocate tbh :)

Copy link
Member Author

Choose a reason for hiding this comment

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

So calling initialize_widget would be the path (super'ing doesn't really work well with how we have UIElement set up). It would completely break anything using widgets, though, which seems like a bummer.

If I can suss out a way to give a more direct warning about the situation, I'd be happier about breaking compatibility. Let me see what I can find.


class << self
attr_accessor :app
def handle_block(*_)
Copy link
Member

Choose a reason for hiding this comment

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

just one thing, do we have redrawing etc. correctly setup for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you mean? Because of where this is called it wires into the startup sequence correctly, and after that it's just a slot so everything's happy.

class Widget
include Common::Inspect

class Widget < Shoes::Flow
Copy link
Member

Choose a reason for hiding this comment

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

why do we inherit from flow and not Slot? Cause flow is the default, makes sense, just checking though

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked the behavior on the old Widgets, and they acted like a Flow.

@jasonrclark
Copy link
Member Author

Closing this in favor #1476

@jasonrclark jasonrclark closed this Oct 1, 2017
@jasonrclark jasonrclark deleted the widget branch October 1, 2017 16:17
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