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 Box hierarchy for GTK4 #313

Merged
merged 2 commits into from Aug 27, 2023
Merged

Fix Box hierarchy for GTK4 #313

merged 2 commits into from Aug 27, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 16, 2023

What is this supposed to fix?

Running the following code should result in a window with a button with a box, but only the window shows.

Snippet: GTK4 window with a Box

lgi = require 'lgi'
gtk = lgi.require('Gtk','4.0')

local app= gtk.Application{application_id = 'org.phicross.gtk4lua.intro'}
local handles = {application = app}

function app:on_startup()

end

function app:on_activate()
    handles.window = gtk.ApplicationWindow{ id = 'daWindow',
        application = self,
        title = "intro app.",
        default_width = 200,
        default_height = 200
    }

    handles.atop = gtk.Box{ orientation = gtk.Orientation.VERTICAL,
        id='daBox',
        gtk.Button {
            id = 'daButton',
            label = 'slap',
            on_clicked = function(self)
            print 'did you just slap me?'
            end
        } 
    }
    handles.window:set_child(handles.atop)

    print 'app ready.'
    print (handles.window.id)
--     print (handles.window.child.daButton.label)
--     print ('app window box child is '..handles.atop.child.name)

    handles.window:show()
end

app:run(arg)

Using the GTK4 interactive debugger with <CTRL>+<SHIFT>+I , you can browse through the widget hierarchy, where the ApplicationWindow object will have a Box child, but the Box will not have anything in it.

Running the code with this fork's LGI will result in the hierarchy properly rendering, with a labeled button that says 'slap'.

How was this fixed?

Just a single line on the GTK override file, where i set the underlying _container_add gobject method of the Box type to Box.append().

Are you seriously gonna publish just this naiive change to the main fork?

This pull request is for testing the waters. It is likely that more underlying code must be overriden for id navigation (e.g. handles.atop.daBox.daButton.label='press me'), child-on-child, other Container-like widgets that are no longer containers as of Gtk4, etc.
Is there demand for this library with Gtk4? are people gonna use lua constructors instead of the language-independent builder XML format? what kind of knowledge on lgi's internals do i need to accomodate for GTK4's API breakage? I would very much like a little catching-up on this matter.

@ghost ghost changed the title Make Gtk Box call append() for array part Fix Box hierarchy for GTK4 Aug 16, 2023
@psychon
Copy link
Collaborator

psychon commented Aug 17, 2023

I have no clue about this, but I have the powers to merge this. Is there anyone with some clue wanting to chime in?

Also, should "something" be added to the test suite for this? (I don't know what / how to test this; I am just thinking out loud.)

@@ -219,6 +219,9 @@ if Gtk.Container then
end
end

-------------------------------- Gtk.Box overrides.
Gtk.Box._container_add = Gtk.Box.append

Choose a reason for hiding this comment

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

Do we need to wrap this with a GTK version check to only apply to GTK >= 4?

Copy link
Author

Choose a reason for hiding this comment

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

Good idea. This method doesn't exist in GTK3. I'll look into moving this down the bottom the file inside a "GTK4 overrides" block, right next to the old GTK2 check.

Copy link
Author

@ghost ghost Aug 19, 2023

Choose a reason for hiding this comment

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

as of the latest commit:

Gtk override

-------------------------------- Gtk-4 overrides

if Gtk._version == '4.0' then

   --- CONTAINER CLASSES
   --- GTK4 Has removed the Container abstract class.
   --- This means Boxes, Grids and other layout widgets 
   --- Must now add children using their own specific methods.

   --- Gtk.Box overrides

   --- widgets on a the array part of the constructor of a Box
   --- will be `append()`-ed
   --- in order of appearance.
   Gtk.Box._container_add = Gtk.Box.append

end

This will prevent Gtk versions ~= 4.0 from attempting Gtk.Box.append().

@ghost
Copy link
Author

ghost commented Aug 19, 2023

Container override substitute for GTK4

It might be possible to reuse the same logic from the Container class override to bring nested widget constructors back without duplicating the whole thing for each class. If i get it working, i'll make another PR.

@Aire-One
Copy link

I think we can take a look at how other Language Bindings are doing, https://www.gtk.org/docs/language-bindings/index.

GJS and PyGOject seems to be good references: https://gitlab.gnome.org/GNOME/gjs/-/blob/master/modules/core/overrides/Gtk.js https://gitlab.gnome.org/GNOME/pygobject/-/blob/master/gi/overrides/Gtk.py

According to https://gitlab.gnome.org/GNOME/pygobject/-/merge_requests/148 and https://gitlab.gnome.org/GNOME/pygobject/-/merge_requests/150, PyGObject took the decision to keep a "phantom class" for Container (I'm not sure to correctly understand what the second MR changed in this regard).

GJS seems to use JavaScript prototype magic 🤷 https://gitlab.gnome.org/GNOME/gjs/-/commit/bdb66678e0b09802747767e40f66208c2a02e359 and https://gitlab.gnome.org/GNOME/gjs/-/commit/2ede057f18c0c6a22b98fe04118ff06304b4a1cb

@ghost
Copy link
Author

ghost commented Aug 21, 2023

So, just so we're on the same page here... is it OK to merge? are we missing something? does it need a corresponding "container methods" test? are we waiting on more reviewers? it this GTK4 progress snippet not enough to warrant a PR? Should i push it to a "GTK-4 WIP" branch instead?
Sorry if if sounds like i'm bombarding questions, i just started playing with this library last friday and i don't know how things usually work on this ten-year-old repo.

@Aire-One
Copy link

No worry's, I did some investigation and and have commented my findings.

The "container thing" can be investigated and sorted out with other PRs. It will probably require a lot of work.

Regarding tests, I'm not sure how to proceed, and we probably don't have the necessary tooling right now.

I think we can merge. (I can't do it tho)

@psychon
Copy link
Collaborator

psychon commented Aug 27, 2023

i just started playing with this library last friday and i don't know how things usually work on this ten-year-old repo.

It works like this: The original maintainer disappeared and since then, things basically do not work. 🤷

@psychon psychon merged commit e929060 into lgi-devs:master Aug 27, 2023
5 checks passed
@ghost ghost mentioned this pull request Aug 28, 2023
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.

2 participants