markqvist / NomadNet

Communicate Freely
GNU General Public License v3.0
1.17k stars 44 forks source link

Allow newer versions of urwid #46

Closed penguinolog closed 8 months ago

penguinolog commented 8 months ago

Last urwid versions are backward-compatible with 2.1.2 (but may produce "DeprecationWarning" on legacy logic)

markqvist commented 8 months ago

Did they release updates that ensured backward-compatibility? Because the first 2.4.0 release was definitely not backwards compatible, and totally broke everything, which is why I pinned it at 2.1.2.

markqvist commented 8 months ago

Ah, I see in the Urwid changelog you actually fixed a lot of stuff over there :) I'll take it you know what you're talking about then, and try the updated Urwid versions! If all checks out, I can easily bump it up to the latest and release a new build.

penguinolog commented 8 months ago

Thank you. 2.4.0 was broken due to import hell on testing. By dev-iinstall and install from requirements I ended with 2 versions of urwid installed in 1 environment. After release and re-test, tests failed -> 2.4.0 was fixed in 2.4.1 and version 2.4.0 was yanked to prevent install by end users.

markqvist commented 8 months ago

Aha, thanks for the info! That explains why nothing was working when I initially tried it out, then! I'll have this up soon, thanks for all the work you did on this!

markqvist commented 8 months ago

Did something change regarding default sizing for widgets? For example, in 2.1.2, this pile widget has sizing:

frozenset({'box', 'flow'})

Which is what I'd expect, and necessary for pages to render correctly. But in 2.4.2, the same pile widget now has sizing:

frozenset({<Sizing.FIXED: 'fixed'>, <Sizing.FLOW: 'flow'>})

Which obviously means that every text line will get to render as many columns as it needs, messing up rendering within scrollable widgets:

urwid

This breaks the main functionality of nomadnet, and while I have been trying to figure it out for an hour and a half now, I cannot for the life of me understand why it gets fixed sizing. Setting box_columns on the Columns widget, or setting box_widget=True in the column options when inserting the new content does nothing.

Do you have any ideas, thoughts or pointers?

penguinolog commented 8 months ago

Historically sizing was hard coded, which ended that mark was almost useless (crash in fixed widgets, not supported flow while marked, trying to render flow as fixed). Now sizing is calculated from contents. Easier to check "sizing" method do string for rules and partial explanation for source of size information

Op ma 15 jan 2024 14:32 schreef markqvist @.***>:

Did something change regarding default sizing for widgets? For example, in 2.1.2, this pile widget https://github.com/markqvist/NomadNet/blob/master/nomadnet/ui/textui/Guide.py#L167 has sizing:

frozenset({'box', 'flow'})

Which is what I'd expect, and necessary for pages to render correctly. But in 2.4.2, the same pile widget now has sizing:

frozenset({<Sizing.FIXED: 'fixed'>, <Sizing.FLOW: 'flow'>})

Which obviously means that every text line will get to render as many columns as it needs, messing up rendering within scrollable widgets https://github.com/markqvist/NomadNet/blob/master/nomadnet/vendor/Scrollable.py :

urwid.png (view on web) https://github.com/markqvist/NomadNet/assets/1438012/163dd82d-b20e-43c4-9f73-9f7fa7db7648

This breaks the main functionality of nomadnet, and while I have been trying to figure it out for an hour and a half now, I cannot for the life of me understand why it gets fixed sizing. Setting box_columns on the Columns widget, or setting box_widget=True in the column options when inserting the new content does nothing.

Do you have any ideas, thoughts or pointers?

— Reply to this email directly, view it on GitHub https://github.com/markqvist/NomadNet/pull/46#issuecomment-1892184570, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD2KMMZGONGBGGE2C43BIK3YOUVXBAVCNFSM6AAAAABB27GT6WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJSGE4DINJXGA . You are receiving this because you authored the thread.Message ID: @.***>

penguinolog commented 8 months ago

All updated widget calculation mark maximum supported combinations, so if not marked as box - old code raised exception while trying to handle as box. Opposite: if marked as fixed - widget can calculate size to fit everything (but not forces to render as fixed)

markqvist commented 8 months ago

Thanks, but how do I make sure the pile and its contents are treated as box/flow widgets, then?

penguinolog commented 8 months ago

If pile contains box - it can be always rendered as box, but also can be rendered as flow if all box widgets GIVEN. Pile is always can be rendered as flow if flow widgets present (GIVEN flow-only is wrong case for pile) Pile can be handled as fixed only if it contains fixed widgets in pack or fixed+box/flow WEIGHT.

penguinolog commented 8 months ago

Small remark: if at least 1 box-only widget not GIVEN - pile can not be rendered as Flow: no source of height for such widget.

penguinolog commented 8 months ago

http://urwid.org/reference/widget.html#urwid.Pile.sizing Doctests was bad handled by sphinx

markqvist commented 8 months ago

Since the pile is inside a Columns, where width of the columns is set with weight, should that not automatically make the pile inside render as box?

The goal here being that text is wrapped in the page viewer, and doesn't just flow on beyond the end of the screen forever, like in the image I posted above.

This is the correct way to render it of course: urwid2

This is how it renders now with 2.4.2: urwid

markqvist commented 8 months ago

Screenshot of the full layout, rendered by 2.1.2:

urwid3

penguinolog commented 8 months ago

Can you point me the location for this test? I'll look at it when I'll get access to the computer

markqvist commented 8 months ago

Ok, thanks a lot, with those things in mind I was actually able to make some progress here, and it's probably all mostly to do with how the Scrollable implementation I was using was expecting sizing parameters order. I'm going to test this out a bit more, and hopefully it is all good.

On the upside I may also have a Scrollable and Scrollbar class that works bug-free and has proper navigation and cursor handling that can be included in Urwid core, unless you already have something lined up there.

penguinolog commented 8 months ago

Small check output (both urwid < 2.2 and last):

>>> w = urwid.Pile((urwid.Text("line " + str(line)) for line in range(10)))
>>> w.render((10,10))
Traceback (most recent call last):
...
ValueError: too many values to unpack (expected 1)

On the last urwid:

>>> w = urwid.Pile((urwid.Text("line " + str(line)) for line in range(10)))
>>> w
<Pile fixed/flow widget>
>>> w.sizing()
frozenset({<Sizing.FIXED: 'fixed'>, <Sizing.FLOW: 'flow'>})
>>> columns = urwid.Columns((w, (2, urwid.SolidFill("|"))), box_columns=(1,))
>>> columns
<Columns widget>
>>> columns.sizing()
frozenset({<Sizing.BOX: 'box'>,
           <Sizing.FIXED: 'fixed'>,
           <Sizing.FLOW: 'flow'>})
>>> print(columns.render(()))
line 0||
line 1||
line 2||
line 3||
line 4||
line 5||
line 6||
line 7||
line 8||
line 9||

In version 2.4.2 looks like bug present: Columns can not be rendered smaller than Flow widget. In version 2.1.2 Columns can not be rendered as BOX (and without "box_columns" can not be rendered at all):

ValueError: too many values to unpack (expected 1)
markqvist commented 8 months ago

Thanks for all the info and pointers @penguinolog. It's all working fine on 2.4.2 now.

In relation to https://github.com/urwid/urwid/issues/226, maybe you're interested in merging Scrollable.py from nomadnet into main Urwid.

It is originally from @rnduser, but I fixed some of the small bugs that it had here and there, and improved navigation and cursor placement on page up/down and mousewheel navigation.

penguinolog commented 8 months ago

That will be great, if you will make such PR! At this moment is no official "scrolling" widget and huge demand for it. Only 1 serious issue present: urwid if LGPL, not GPL and re-licensing should not happen

markqvist commented 8 months ago

I'll be happy to make such a PR, and as for the code changes and improvements I made to Scrollable, I am fine with contributing that under LGPL, so if @rndUsr is also okay with doing that, we are good to go!

rndusr commented 8 months ago

Not a fan of the LGPL, but because I still rely on urwid daily and because I'm happy to see it come back to live, I hereby LGPL it for urwid.

Also because you asked nicely. :)

markqvist commented 8 months ago

Thanks a bunch @rndusr! And thanks for writing it in the first place :)