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

Set GTK 4 decoration layout #5421

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

jexposit
Copy link
Contributor

@jexposit jexposit commented Jan 24, 2024

The current attempt to show only the close button in the external application windows (see org.gnome.desktop.wm.preferences.gschema.override, button-layout property) is not working.

Instead, add a configuration file for GTK 4.


After spending quite a lot of time trying to make .gschema.override files work I gave up and decided to follow this approach.

I'm not sure if including this configuration file is acceptable or not.

Note that I had to set the environment variable XDG_CONFIG_DIRS (os.environ['XDG_CONFIG_DIRS'] = xdg_config_dirs) instead of settings it in util.startProgram(env_add=...). Otherwise, child processes, like Teclas, won't use the right paths.

I wonder if XDG_CONFIG_DIRS should be set in augmentEnv() instead. Waiting for feedback:

def augmentEnv():
    env = os.environ.copy()

    # Set XDG_CONFIG_DIRS so GTK 4 preferences are used
    datadir = os.environ.get('ANACONDA_DATADIR', '/usr/share/anaconda')
    xdg_config_dirs = datadir
    if 'XDG_CONFIG_DIRS' in os.environ:
        xdg_config_dirs = datadir + ':' + os.environ['XDG_CONFIG_DIRS']
    env.update({"XDG_CONFIG_DIRS": xdg_config_dirs})

    # FIXME: Remove the support for the ANA_INSTALL_PATH variable.
    env.update({"ANA_INSTALL_PATH": conf.target.system_root})
    env.update(_child_env)
    return env

As a side note, I also tried to follow the same approach with XDG_DATA_DIRS, but it didn't fix the issues.

@jexposit
Copy link
Contributor Author

/build-image --boot.iso

Copy link

Images built based on commit 57a7109:

  • boot.iso: success

Download the images from the bottom of the job status page.

@M4rtinK
Copy link
Contributor

M4rtinK commented Jan 25, 2024

After spending quite a lot of time trying to make .gschema.override files work I gave up and decided to follow this approach.

I'm not sure if including this configuration file is acceptable or not.

If I understand it correctly, its stored in /usr/share/anaconda & won't be really loaded unless this folder is added to the correct environment variable, right ?

That way it should really only influence the installation environment on the boot.iso and perhaps applications started by the Anaconda process on Live images. That I think should be safe and potentially even desirable in other cases, say for the network-connection-editor, if it switches to GTK4 or if we end up launching more GTK4 apps.

Copy link
Contributor

@M4rtinK M4rtinK left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! :)

The current attempt to show only the close button in the external app
windows (see org.gnome.desktop.wm.preferences.gschema.override,
button-layout property) is not working anymore.

Instead, add a "settings.ini" configuration file for GTK 4 in
"/usr/share/anaconda/gtk-4.0" and add this path to the XDG_CONFIG_DIRS
environment variable so it is used.

For more details about how this file is used, see:
https://docs.gtk.org/gtk4/class.Settings.html
@jexposit
Copy link
Contributor Author

Thanks for you review Martin.

If I understand it correctly, its stored in /usr/share/anaconda & won't be really loaded unless this folder is added to the correct environment variable, right ?

Yes. I added more details to the commit description so it is clear how this work.

Removing the "draft" tag, but I don't know if we should set the environment variable in augmentEnv() instead. Letting you decide on this one.

@jexposit jexposit changed the title Draft: Set GTK 4 decoration layout Set GTK 4 decoration layout Jan 26, 2024
@M4rtinK
Copy link
Contributor

M4rtinK commented Jan 26, 2024

/build-image --live

Copy link

Images built based on commit c6e72e6:

  • Live: failure

Download the images from the bottom of the job status page.

@M4rtinK
Copy link
Contributor

M4rtinK commented Jan 26, 2024

/build-image --live

@M4rtinK
Copy link
Contributor

M4rtinK commented Jan 26, 2024

Just checking the boot.iso (as live image creation seems to have some issues right now) and all seems fine:
tecla_gtk4_theme
I've also checked some other "on top" cases, which are not GTK4 based, just in case.
dialog_gtk4_theme
A modal dialog.
connection_editor_gtk4_theme
The network connection editor (GTK3 ?).

Both look fine. :)

Copy link

Images built based on commit c6e72e6:

  • Live: failure

Download the images from the bottom of the job status page.

@M4rtinK
Copy link
Contributor

M4rtinK commented Jan 30, 2024

/build-image --boot.iso --live --webui

Copy link

Images built based on commit c6e72e6:

  • boot.iso: success

  • Live: success

Download the images from the bottom of the job status page.

@M4rtinK
Copy link
Contributor

M4rtinK commented Jan 30, 2024

/build-image --live

Copy link

Images built based on commit c6e72e6:

  • Live: success

Download the images from the bottom of the job status page.

@M4rtinK
Copy link
Contributor

M4rtinK commented Jan 31, 2024

Removing the "draft" tag, but I don't know if we should set the environment variable in augmentEnv() instead. Letting you decide on this one.

Hmm, that could be also a possibility, but I think I like it better how it is co-located with other GUI/display related things, so I suggest we keep it as is. We can always switch to augmentEnv() if we find out the current way is causing issues, but it seems to work fine right now according to my testing.

Feel free to merge the PR. :)

@jexposit
Copy link
Contributor Author

Thanks for your review Martin. Unfortunately, I can't merge it:

image

@M4rtinK M4rtinK merged commit a721090 into rhinstaller:master Feb 1, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants