Ruin0x11 / OpenNefia

(Archived) Moddable engine reimplementation of the Japanese roguelike Elona.
MIT License
115 stars 18 forks source link

Consider redesigning how `:relayout()` works as its functionality can be redundant #143

Open Ruin0x11 opened 3 years ago

Ruin0x11 commented 3 years ago

Why is it necessary to do this in some situations?

function MyElement:relayout()
   for j = 1, 10 do
      local x = self.x + 60
      local y = j * 18
      self.icons[i]:relayout(x, y)
   end
end

function MyElement:draw()
   for j = 1, 10 do
      local x = self.x + 60
      local y = j * 18
      self.icons[i]:draw()
      Draw.text("Text " .. j, x + 20, y)
   end
end

The problem is that Draw.text() is stateless, while icons[i] is a stateful UiElement. You could just call relayout(x, y) in the draw() method, but then why would we even call it in the parent element's relayout() as above? It sounds leaky to me.

Maybe we should pass x and y to IElement:draw(x, y) and only leave IUiLayer:draw() as a 0-argument method (or maybe a two-argument method where you can supply an optional x/y pair that overrides self.x/self.y?). Then the IUiLayer would become the only GUI component to have knowledge of its own position/size.

function MyElement:relayout()
end

function MyElement:draw(x, y)
   for j = 1, 10 do
      local x = self.x + 60
      local y = j * 18
      self.icons[i]:draw(x, y)
      Draw.text("Text " .. j, x + 20, y)
   end
end

Could still be somewhat leaky when it comes to IUiLayers drawing/updating nested IUiLayers, as some of the code already does. You'd call :relayout() on the child layers in the parent's :relayout() still, and call :draw() with no arguments like before.

I don't think it makes sense to have an IUiElement draw/update an IUiLayer. Kind of useless because the IUiElement won't be able to send input to its sublayer, unless it implements IInput.

Ruin0x11 commented 3 years ago

Or in this case you could just use Draw.make_text() inside :relayout() instead of calling Draw.text() in :draw() if you have some stateful elements that you want to relayout all at once.