Title/status bar unset still take up UI space

[lenormf]

Describe the bug

When unsetting the title/status bar objects, some space is still taken up on the UI where they would have otherwise been displayed.

To Reproduce
Steps to reproduce the behavior:

1. Set the status_bar and title_bar attributes of the PyCUI instance to None
2. Start the UI

Expected behavior
The widgets are displayed starting/ending at the top/bottom of the terminal, not one row away (where the bars would be display).

[lenormf]

Related #116

[PabloLec]

Maybe we could add a hide() method for status_bar and title_bar.

But from what I understand, top and bottom padding are absolute.

I guess top padding is set in Widget.get_absolute_start_pos() with:

y_pos = self._row * row_height + 2 + y_adjust

And bottom padding is set within UIElement.get_viewport_height() with:

return self._height - (2 * self._pady) - 3

We could either replace these absolute values by relatives or use if-statements based on potential _hidden = True attributes.

[jwlodek]

Yes, at the moment the padding is hard-coded both in those functions as well as some other places where the logic for window re-sizing is located. Something like

height = height - 4

which makes the window height subtract the padding for the title/status bars. Probably this can be changed to add a check to see if one or both are hidden or not.

[PabloLec]

Indeed after looking closer I see some more hard-coded values 😬

So, there are multiple height - 4, in init.pyand widget_set.py like so:

self._height                = self._height - 4

Also references to the bottom 3 padding as in popups.py:

viewport_height = self._height - (2 * self._pady) -  3

If I understand correctly, the 3 is for 2 blank lines + 1 line for status bar?

And I see other absolute values in renderer.py, widget_set.py, widget.py or slider.py

I can work on it, I was thinking about adding to StatusBar something like this:

class StatusBar:
     def __init__(self):
          self.__visible = True
          self.__height = 1

     @property
     def height(self):
         return self.__height

     @property
     def visible(self):
         return self.__visible
       
     @visible.setter
     def visible(self, value: bool):
         if not type(value) == bool:
            raise ValueError
         self.__visible= value
         if self.__visible:
            self.__height = 1
         else:
            self.__height = 0

But then we still have to deal with the 2 bottom blank lines padding. Either we let these 2 lines, which might seems odd depending on the user needs. We could replace these 2 lines by a single line when there is no status bar, to be even with top padding. Maybe for this case if status_bar.visible: foo; else: bar could do the job?

Also, maybe visible is not the best attribute name? show? display?

[PabloLec]

@jwlodek You can assign me on this one.

Can you just tell me what do you think about these above attributes/methods for StatusBar?

Also, if I understand correctly, there are currently respectively 1 and 2 blank lines at the top and bottom, separating the status bars from other UI content. How should we handle that if the status bars are hidden? Either just remove these lines or leave a 1 line padding on both sides?

[jwlodek]

So for the first part of your previous comment, the height - 4 is basically the whole window height, minus the title/status bar, minus two extra for padding on top and on bottom. Also, in y_pos = self._row * row_height + 2 + y_adjust, the two references the titlebar, and then the padding line, since y-positions start at the top of the terminal. The other instance you referenced from the popups I'm pretty sure doesn't actually have anything to do with the status/title bars, instead it is referring to the border around the widget or popup. The viewport_height is the height in rows inside the widget not counting the borders.

For the StatusBar attributes, is there a reason we need a height attribute? I think just adding a conditional where the height is set to add 1 if visible is true would be enough.

Off of the top of my head, here are the places where this padding is hard-coded:

• py_cui/py_cui/__init__.py

  Line 137 in 46bbbf9

  self._height = self._height - 4

• py_cui/py_cui/__init__.py

  Line 283 in 46bbbf9

  height = height - 4

• py_cui/py_cui/__init__.py

  Line 1612 in 46bbbf9

  height = height - 4

• py_cui/py_cui/widget_set.py

  Line 53 in 46bbbf9

  self._height = self._height - 4

• py_cui/py_cui/widgets.py

  Line 160 in 46bbbf9

  y_pos = self._row * row_height + 2 + y_adjust

I could have missed something somewhere, but pretty sure those are the only ones.

Most of the other hard-coded values are relative to the height/width that is centrally managed, so they should stay as they are for now. Most are just used to handle the borders.

I'd say maybe we should have the 1 character top/bottom padding as an optional thing, have it enabled by default and add a toggle method for it. And maybe have it togglable whether the status/title bars are enabled or not

[PabloLec]

Ok, thanks for the help👍

I think we need a StatusBar height attribute to allow relative value attribution like so:

self._height                = self._height - self.status_bar.height - self.title_bar.height - 2

Avoiding multiple ifs, considering we could leave only one of the two status bars visible, that would already be 4 possible combinations.

And if we do set a padding toggle for these top/bottom lines, first we need a good name😉 And, then I think we also should have a var to store that 2 or 0. Or, for this case, we accept to add if-statements but I think it will be useless repetitions where we could only declare the value once.

[PabloLec]

Alright I've tinkered a bit, trying to only consider status bars, not the padding blank lines yet.

First, I came up with this type of implementation to show/hide the status bars:

class StatusBar:
    def __init__(self, text, color):
        self.__height = 1

    def get_height(self):
        return self.__height

    def show(self):
        self.__height = 1

    def hide(self):
        self.__height = 0

I think it's more consistent with current coding style.
So title_bar and status_bar would have .show() and .hide() methods.
And their height attribute would be accessed like so:

self._height = self._height - self.title_bar.get_height() - self.status_bar.get_height() - 2

So, right now, modifying __init__.py's - 4 operations is easy.

Regarding widget_set.py, it currently has no access to root so there is no possibility to grab status bars height. The only way to do so is by adding a status_bars_height parameter. Not a big deal I guess, as it is rarely initiated.

Now, what's trickier:

• In widgets.py

y_pos = self._row * row_height + 2 + y_adjust

We could grab the status bars using self._renderer._root.status_bar but when the widgets are initiated, their renderer attribute is None, and then attributed via _assign_renderer UI method. So again, no possibility to grab status bars height at instantiation.
What we could do:
1- Either pass the renderer as an argument at instantiation.
2- Or overload UI _assign_renderer method to call update_height_width widget method.
I prefer number 2.

• In __init__.py

Main PyCUI object _grid attribute is created at init. Regular, with status bars, height is passed as an argument as lines like root.status_bar.hide() may only appear later in the user's script.
So, the terminal has to be resized for the renderer to redraw the grid with updated height. A nice and easy solution would have been to invoke Grid.update_grid_height_width() method during StatusBar.hide() but StatusBar object has no access to root, renderer or grid.
Here I'm not quite sure what would be the best thing to do. Probably again adding a parameter, maybe just passing PyCUI as root.

[jwlodek]

I think this implementation suits the current code style for the project better like you said, so I think that we should keep. For the widget set, I think just having an extra parameter (the root PyCUI instance) passed in makes sense, since each widget set should be tied to the overall UI. I'm eventually planning to have the base PyCUI object be an extension of the widgetset class, to avoid a bunch of the code duplication we have now.

For the widgets, I think maybe the best solution is to have it be done through the grid, since each widget already gets a reference to the grid it inhabits. That's another thing I want to revisit in the future is adding support for different layout managers than just a grid, but for now I think having the grid handle that kind of resizing would be best. And I think passing a reference to root to the statusbar objects would probably be the easiest way to accomplish this

[jwlodek]

I believe this issue has been resolved on the v0.1.4 branch, in PR #125 . Thanks @PabloLec !