wwmm / easyeffects

Limiter, compressor, convolver, equalizer and auto volume and many other plugins for PipeWire applications
GNU General Public License v3.0
6.15k stars 265 forks source link

Plans for EasyEfects 6.2.0 #1243

Closed wwmm closed 2 years ago

wwmm commented 2 years ago

I think it is time to make another structural change. In the last days I started to play with the idea of using gtk4 directly without wrappers(gtkmm4) in the way. I like gtkmm and it has served us well since my initial move away from Python. But there are some things here and there that are not straightforward(or possible) when using gtkmm:

After thinking a little about all of this I created a new branch where I am investigating how things would be if we were using gtk4 directly https://github.com/wwmm/easyeffects/tree/libadwaita. So far so good. I was afraid that I would have to write tons of boilerplate code but with composite templates writing gtk4 directly has been surprisingly simple. No major difficulties or annoyances. At least for now... But I would not say yet that this move to abandon gtkmm is definitely going to happen. It is too soon for that.

Digitalone1 commented 2 years ago

I do not know. Maybe instead of having a switch where prefer light / prefer dark are the only options we should have a GtkComboBox with all the possible values and having this setting at its default value.

I see modern websites having three options: system, light and dark. See this in desktop mode, try to change the theme from the top bar.

A combobox system (default), light (force) and dark (force) should be better.

I do not remember having changed that part of the xml. Maybe this was on the C++ code side.

I don't even remember if we had it vertically centered on gtkmm. I'll give a look at it when have time.

Digitalone1 commented 2 years ago

Schermata del 2021-12-16 22-25-49

Output channel and channel, waveform and tipe, frequency and value... They seem redundant. Maybe better the following:

wwmm commented 2 years ago

A section named Preferences inside a preferences window also seems weird.

wwmm commented 2 years ago

Properties may be better as we are talking about a signal.

wwmm commented 2 years ago

A section named Preferences inside a preferences window also seems weird.

I am not sure why I thought about the preferences window... I must be working too much :smile:. In any case I think Properties is a good choice.

wwmm commented 2 years ago

This is how it looks like after the changes Screenshot from 2021-12-16 19-41-58

Digitalone1 commented 2 years ago

Can't they be gathered like this?

Schermata del 2021-12-16 23-44-07

Also, consider what I proposed about light/dark mode.

wwmm commented 2 years ago

Can't they be gathered like this?

Good question. I will see if it is possible.

Also, consider what I proposed about light/dark mode.

No problems. I agree with the idea.

wwmm commented 2 years ago

New version Screenshot from 2021-12-16 19-53-16

vchernin commented 2 years ago

I saw currently EE is not specifying which libadwaita version to use https://github.com/wwmm/easyeffects/blob/d5865955c5fb5226768cfe3fa1dda6389cf391a1/src/meson.build#L124

Should it specify >= 1.0.0.alpha.4, since that's the version it's tested with? (I hope meson would understand that beta > alpha...)

wwmm commented 2 years ago

Should it specify >= 1.0.0.alpha.4, since that's the version it's tested with? (I hope meson would understand that beta > alpha...)

I tried now dependency('libadwaita-1', version: '>=1.0.0.alpha.4') and Meson did not complain. If it is actually able to detect that something like 1.0.0.alpha.5 or 1.0.0.beta is a newer version is another story. As it is working I will specify it and see what happens when Adwaita 1.0 is finally released.

wwmm commented 2 years ago

The current code in the master branch disconnects filters from PipeWire when they are not needed anymore. So far so good. I did not notice any freezing because of that. But this is something we should test for some time. I may delay the next release to the first week of the next year.

@Digitalone1 and @vchernin when you have time try to do some tests. The last time I tried to disconnect filters when they are not needed anymore there were lots of random freezes. But somehow now it just worked on the first implementation attempt. Weird... I hope it is really fine to do that now.

Digitalone1 commented 2 years ago

Nice work @wwmm

No issues to me. I'll keep testing.

wwmm commented 2 years ago

We now have a shortcuts window Screenshot from 2021-12-29 13-05-32 We do not have much to show. But without it most users won't notice that we now can go to fullscreen.

vchernin commented 2 years ago

@wwmm libadwaita 1.0 and GTK 4.6 were released. Arch seems to already have them. Hopefully everything is still in good shape.

wwmm commented 2 years ago

libadwaita 1.0 and GTK 4.6 were released. Arch seems to already have them. Hopefully everything is still in good shape.

Meson complained about the alpha tag

easyeffects/src/meson.build:121:0: ERROR: Invalid version of dependency, need 'libadwaita-1' ['>=1.0.0.alpha.4'] found '1.0.0'.

But that was easy to fix. I have already updated the master branch

wwmm commented 2 years ago

I am doing some tests and things seems fine. It is a little disappointing that gtk 4.6.0 does not fix https://gitlab.gnome.org/GNOME/gtk/-/issues/4369. But at least no new issue seems to have been created by it. At least on Wayland.

Digitalone1 commented 2 years ago

@wwmm notify me when it's ready for release, I will update the translation.

Next step towards 7.0 would be the multiple instances of plugins.

I wanted to work also on new pitch plugin exposing the hidden features, but maybe in the next months when I have more free time.

wwmm commented 2 years ago

notify me when it's ready for release, I will update the translation.

You can do that now if you have time. It is a good moment to do a new release. I am taking a look at #1211 but GtkScale seems to have the bindings hardcoded in its sources. We may have to reconsider the volume slider orientation to fix that problem... So it is better to let this for another moment in the future.

wwmm commented 2 years ago

Next step towards 7.0 would be the multiple instances of plugins.

And in order to do that we will have to do some serious rework in two files

https://github.com/wwmm/easyeffects/blob/master/src/effects_base.cpp
https://github.com/wwmm/easyeffects/blob/master/src/plugins_box.cpp

The map with the filters in effects_base.cpp will have to be populated on the fly instead of creating all the filters classes in the constructor in the initialization phase. And that will probably require some changes in plugins_box.cpp because the filter classes have to be alive by the time the graphical interface of each plugin tries to bind to their signals.

In any case it is something that should be discussed in a Plans for EasyEffects 7.0 issue.

vchernin commented 2 years ago

Not sure if this is a Flatpak-specific problem, but I'm getting:

(easyeffects:2): Gdk-DEBUG: 12:37:32.465: Ignoring portal setting for org.gnome.desktop.wm.preferences auto-raise-delay
(easyeffects:2): Gdk-DEBUG: 12:37:32.465: Ignoring portal setting for org.gnome.desktop.wm.preferences audible-bell
(easyeffects:2): Gdk-DEBUG: 12:37:32.465: Ignoring portal setting for org.gnome.desktop.wm.preferences visual-bell
(easyeffects:2): Gdk-DEBUG: 12:37:32.465: Using portal setting for org.gnome.fontconfig serial: 0

(easyeffects:2): GLib-CRITICAL **: 12:37:32.472: g_hash_table_lookup: assertion 'hash_table != NULL' failed

(easyeffects:2): GLib-CRITICAL **: 12:37:32.487: g_hash_table_lookup: assertion 'hash_table != NULL' failed
(easyeffects:2): Adwaita-DEBUG: 12:37:32.510: Setting org.freedesktop.appearance.color-scheme of type u not found
(easyeffects:2): Adwaita-DEBUG: 12:37:32.511: Setting org.gnome.desktop.interface.color-scheme of type s not found
(easyeffects:2): Adwaita-DEBUG: 12:37:32.513: Setting org.gnome.desktop.interface.a11y.high-contrast of type b not found

(easyeffects:2): GLib-CRITICAL **: 12:37:32.513: g_hash_table_lookup: assertion 'hash_table != NULL' failed

at startup. The Glib critical warnings show without G_MESSAGES_DEBUG=all.

wwmm commented 2 years ago

Usually I set only G_MESSAGES_DEBUG=easyeffects but I tried G_MESSAGES_DEBUG=all now. No critical message on my computer. So it must be related to Flatpak somehow.

vchernin commented 2 years ago

By bisect this is caused by https://gitlab.gnome.org/GNOME/gtk/-/commit/49589e1da180a0070124ca69234673e18ce1f635, which matches the debug messages. I will report it later to gtk.

Digitalone1 commented 2 years ago

In any case it is something that should be discussed in a Plans for EasyEffects 7.0 issue.

Sure, but also a new JSON format of the plugin preset will be needed.

wwmm commented 2 years ago

Sure, but also a new JSON format of the plugin preset will be needed.

It will depend on how we will handle the multiple instances. I think there is a way to implement them without having to change the presets file format again. I have been thinking about something like this

"plugins_order": [
            "bass_enhancer#0",
            "compressor#0",
            "exciter#0",
            "compressor#1",
            "autogain#0",
        ]

Each instance would have an "unique id number" that we could get splitting strings based on the # character. That number would have nothing to do with the plugin position in the pipeline. That would still be given by its position in the plugins_order array. The id numbers would help to associate the plugins instance with the right GSettings path in the database.

The downside of this approach is needing to split strings. In json we can have an array of pairs contaning the id number and the plugin name. But I do not think we can do something like this in GSettings. I will take a look at this. But if we follow this path I agree we would have to change the format again.

wwmm commented 2 years ago

But I do not think we can do something like this in GSettings

But even if there is it would not help when saving the instance settings because we can not have multiple sections with the same name in the json file. With the # approach we can simply saving them like


"limiter#2": {
...
}
wwmm commented 2 years ago

Now that the release was done we can close this issue. Time to think about the plans for the next big one :-)

ShayBox commented 2 years ago

Right now we use GSettings to handle presets and settings in general and while some people may not like it I do not see anything better in the QT ecosystem.

QSettings btw

wwmm commented 2 years ago

QSettings btw

I used it once in one of my personal projects where I used QT. Useful but not better than GSettings.

kkofler commented 1 year ago

In some situations it makes sense to put widgets in menus and this is not the kind of thing that seems doable in QT. Or at least not as straightforward as it is in gtk.

It is actually very straightforward, just create a QWidgetAction wrapping the widget and add that action to the menu. This also works (and is more commonly done) for toolbars, but it can also be done in any menu, except of course the ones displayed remotely through the D-Bus menu spec (in particular, the context menus for indicators = status notifier icons = system tray icons).

Right now we use GSettings to handle presets and settings in general and while some people may not like it I do not see anything better in the QT ecosystem.

Have you looked at kf5-kconfig?

  • Personally I am already used to handle translations in gtk. Relearning to do something so unfun in the QT way would be annoying.

You can easily use gettext with Qt, just use kf5-ki18n (and hopefully soon kf6-ki18n for Qt 6).

wwmm commented 1 year ago

It is actually very straightforward, just create a QWidgetAction wrapping the widget and add that action to the menu. This also works (and is more commonly done) for toolbars, but it can also be done in any menu, except of course the ones displayed remotely through the D-Bus menu spec (in particular, the context menus for indicators = status notifier icons = system tray icons).

Interesting. As I have never seen a QT application doing that it felt like it was not possible. Good to know.

Have you looked at kf5-kconfig?

I looked at it at the time. I have no idea about how it is today but it did not feel an improvement over GSettings. Specially the part about having to generate c++ files from xml https://api.kde.org/frameworks/kconfig/html/kconfig_compiler.html. And even today it is not clear to me if the settings can be changed by third party tools like it is possible with GSettings. Or if there is something similar to GSettings relocatable schemas where you can "mount" the same schema in different paths so that the definitions in the xml can be easily reused.

You can easily use gettext with Qt, just use kf5-ki18n (and hopefully soon kf6-ki18n for Qt 6).

I don't doubt there is a decent way to handle them in QT. KDE is there to prove this. But I would still have to move the few parts of the code that build the translation files to its way of handling things. Not the end of the world. But still not fun.

It is good to see that there are ways to do the same in QT. I will keep this in mind. But they don't feel like better ways. Just different ways. And I would probably still have to actually write a true service instead of using things like the --gapplication-service that comes with gtk. The amount of work involved in a move to QT is huge but the benefits are still questionable in my mind. If I were starting the project from zero today QT would definitely be something to be considered. But with everything already implemented in gtk there isn't much appeal in going through all this trouble.