gtk4-rs icon indicating copy to clipboard operation
gtk4-rs copied to clipboard

[FEATURE REQUEST] Don't re-export dependencies

Open JMS55 opened this issue 2 years ago • 14 comments

gtk-rs (and libadwaita-rs) reexport glib, gio, etc, along with things like the adw/gtk::subclass::prelude module rexporting glib::subclass::prelude. This leads to weird issues where Rust Analyzer has a hard time deciding which import to use, and ends up suggesting a strange one like adw::subclass::prelude::ObjectImpl or gio::subclass::prelude::ObjectSubclass.

In general, I think that we can come up with a better module system than the weird widget/trait/subclass::prelude splits, and things occasionally being inconsistent like the prelude not having everything in the parent subclass module, or the ExtManual traits being in crate::prelude instead of crate::traits, or InitializingObject not being in glib::subclass::prelude.

JMS55 avatar Mar 09 '22 19:03 JMS55

what is crate::traits? that is basically an implementation detail used for the automatically generated traits. But I guess, there isn't much we can do about it from gtk4-rs itself and needs a broader discussion.

bilelmoussaoui avatar Mar 09 '22 19:03 bilelmoussaoui

Not sure what can be done about this, the macros depend on having glib public, or at least parts of it anyway. Any of those traits that we refer to in a macro will need to be re-exported somehow. Would love to be able to avoid that though...

jf2048 avatar Mar 10 '22 00:03 jf2048

I would be open to hide them away in a module called _private or something, that should actually fix some of the issues with proc-macro-crate. Don't know if that would have any effect on rust-analyzer though

jf2048 avatar Mar 11 '22 02:03 jf2048

Quick side note: maybe this issue should be moved to gtk-rs-core or gir or somewhere more appropriate.


I opened a rust-analyzer issue to prioritize imports based on the crate that they were originally defined in https://github.com/rust-analyzer/rust-analyzer/issues/11698. That should help for part of it.


As an example, here is the imports for subclass I made (ignoring non-gtk crates, and after manually fixing the items that RA tried to import weirdly):

use adw::subclass::prelude::AdwApplicationWindowImpl;
use adw::{TabBar, TabPage, TabView, WindowTitle};
use gio::{ActionGroup, ActionMap};
use glib::subclass::prelude::{ObjectImpl, ObjectSubclass};
use glib::subclass::types::InitializingObject;
use glib::{clone, object_subclass, IsA, Object, ObjectExt, ParamSpec};
use gtk::prelude::{GObjectPropertyExpressionExt, InitializingWidgetExt};
use gtk::subclass::prelude::{
    ApplicationWindowImpl, CompositeTemplateCallbacksClass, CompositeTemplateClass, ObjectImplExt,
    ObjectSubclassExt, ObjectSubclassIsExt, TemplateChild, WidgetClassSubclassExt, WidgetImpl,
    WindowImpl,
};
use gtk::traits::{GtkWindowExt, WidgetExt};
use gtk::{
    template_callbacks, Accessible, Application, Buildable, Button, CompositeTemplate,
    ConstraintTarget, Native, Root, ShortcutManager, Stack, Widget, Window,
};

I think on gtk-rs/libadwaita-rs's side, the following re-organization would help:

  • Widgets / Objects / consts / static_functions => gtk/adw/glib/gio (root module)
  • *Ext / *ExtManual => root::traits
  • IsA / Cast / object_subclass / CompositeTemplateCallbacksClass / ObjectSubclass (anything dealing with types, and not the types/methods themselves) => root::meta
  • *Impl => root::impls
  • Do not re-export dependency crates
  • Do not expose sub-modules, such as gtk::widgets, or glib::objects, etc
  • Do not have a prelude module (the crate itself is basically the prelude)
  • If a type from another crate is required to be re-exported for macros (assuming we can't get around this), expose only that specific type in root::_private or something similar.

That would turn my example into the following:

use adw::{TabBar, TabPage, TabView, WindowTitle};
use adw::impls::AdwApplicationWindowImpl;
use gio::{ActionGroup, ActionMap};
use glib::{clone, object_subclass, Object, ParamSpec};
use glib::impls::{ObjectImpl};
use glib::traits::ObjectExt;
use glib::meta::{InitializingObject, ObjectSubclass, IsA};
use gtk::{
    Accessible, Application, Buildable, Button,
    ConstraintTarget, Native, Root, ShortcutManager, Stack, Widget, Window,
};
use gtk::traits::{
    GtkWindowExt, WidgetExt, GObjectPropertyExpressionExt, InitializingWidgetExt, WidgetClassSubclassExt,
    ObjectImplExt, ObjectSubclassExt, ObjectSubclassIsExt
};
use gtk::impls::{ApplicationWindowImpl, WidgetImpl, WindowImpl};
use gtk::meta::{template_callbacks, CompositeTemplate, CompositeTemplateClass, CompositeTemplateCallbacksClass, TemplateChild};

Yes, this requires having all of glib/gio/gtk/etc in your Cargo.toml, which is why the re-exports were originally added. However, I think in hindsight, that that decision has made it more difficult to figure out where items are defined and how to import them, and we should reverse it.

Definitely needs more work, bikeshedding of names, and I'm unsure of the technical details having never used gir, but this is an initial proposal of what I'd like to see as a user. If you're familiar with how the bindings are generated, please weigh in!

JMS55 avatar Mar 13 '22 19:03 JMS55

Yes, this requires having all of glib/gio/gtk/etc in your Cargo.toml, which is why the re-exports were originally added. However, I think in hindsight, that that decision has made it more difficult to figure out where items are defined and how to import them, and we should reverse it.

Definitely don't agree with this, I don't think most people care where a trait comes from

bilelmoussaoui avatar Mar 13 '22 19:03 bilelmoussaoui

I disagree, but we could leave it in. Hiding them from the docs (already done it looks like), and if rust-analyzer becomes smarter about suggesting imports, probably fixes most issues with re-exporting dependency crates.

However, I feel that having more consistent paths is still valuable. Too many things are randomly/only in the prelude/subclass::prelude, or in random modules that aren't consistent with other items.

Regardless of the exact reason, I'm definitely spending a lot of time (mostly when subclassing) tracking down which imports I need, and where I can import them from. I suppose that this could be resolved by a macro like https://github.com/gtk-rs/gtk-rs-core/issues/540.

JMS55 avatar Mar 13 '22 19:03 JMS55

I'm definitely spending a lot of time (mostly when subclassing) tracking down which imports I need, and where I can import them from.

Usually all you need is use gtk::subclass::prelude::*;

Too many things are randomly/only in the prelude/subclass::prelude, or in random modules that aren't consistent with other items.

Any specific examples ? there are probably things we missed or situation we could improve but without concrete situations, it is all just theoretical talking and there is not something actionable to work on.

bilelmoussaoui avatar Mar 13 '22 20:03 bilelmoussaoui

Any specific examples ?

Hard to remember, but I'll keep track of things next time I write a class.

JMS55 avatar Mar 14 '22 00:03 JMS55

A class macro could reduce some of the imports related to ObjectSubclass but probably not much else. All this would still be needed outside the macro:

use adw::subclass::prelude::AdwApplicationWindowImpl;
use adw::{TabBar, TabPage, TabView, WindowTitle};
use gio::{ActionGroup, ActionMap};
use glib::subclass::prelude::{
    ObjectImpl, ObjectImplExt, ObjectSubclassExt, ObjectSubclassIsExt
};
use glib::{class, clone, IsA, Object, ObjectExt};
use gtk::prelude::{GtkWindowExt, WidgetExt};
use gtk::subclass::prelude::{
    ApplicationWindowImpl, TemplateChild, WidgetImpl, WindowImpl,
};
use gtk::{
    Accessible, Application, Buildable, Button, ConstraintTarget,
    Native, Root, ShortcutManager, Stack, Widget, Window,
};

Those traits modules probably shouldn't be public, those are unnecessary re-exports of stuff in the prelude. I wonder if we can just make those pub(crate) mod traits, if rust-analyzer really can't do anything about re-exports. Every extension trait should be in the prelude anyway

jf2048 avatar Mar 14 '22 02:03 jf2048

@jf2048 yes the traits module shouldn't be public at all. It is only used for re-exporting in the prelude. I will fix that on gir's side and do the same for functions

bilelmoussaoui avatar Mar 14 '22 09:03 bilelmoussaoui

BTW disregarding all the widgets, all you have to put at the top is this:

use adw::prelude::*;
use adw::subclass::prelude::*;

Because libadwaita-rs includes all the other preludes.

The only other thing I think could be moved is some stuff in the submodules in glib::subclass:: (not the prelude), those are a little hard to remember

jf2048 avatar Mar 15 '22 00:03 jf2048

The only other thing I think could be moved is some stuff in the submodules in glib::subclass:: (not the prelude), those are a little hard to remember

Indeed, stuff like glib::subclass::Signal but ideally those should no longer be "manually" needed once we have macros for generating them

bilelmoussaoui avatar Mar 17 '22 13:03 bilelmoussaoui

Usually all you need is use gtk::subclass::prelude::*;

This is only even remotely true if all you're using is GTK.

Once you start using libadwaita and gstreamer—in other words, once you start writing a useful application instead of an example—you get really deep into the woods, and you need to go down a Choose Your Own Adventure path:

  • split everything into separate modules and then pay the price of maintaining a complex internal API surface
  • break apart the prelude::* glob and go down an ever increasing atomisation of the use directives
  • give up, and move to another language

ebassi avatar Mar 29 '22 15:03 ebassi

This is only even remotely true if all you're using is GTK.

Once you start using libadwaita and gstreamer [...]

Can you provide an example project where this causes problems? I'm not entirely sure I understand the situation.

There is quite some code out there that imports preludes of GTK, Adwaita and GStreamer inside the same module and that's working fine. Re-exports of the same type/trait are handled as equivalent, no matter where they come from.

The only case where this causes problems is if preludes re-export types (bad idea) or if you have multiple versions of a crate that provides a trait (e.g. glib). In case of the latter you'll run into problems sooner or later anyway as different parts would e.g. disagree what a glib::Object is.

sdroege avatar Mar 29 '22 19:03 sdroege

Closing as there is nothing actionable on the issue itself, feel free to re-open if someone has a proper "broken" use case so we can see what needs to be fixed/improved.

bilelmoussaoui avatar Oct 08 '22 12:10 bilelmoussaoui

On 29 March 2022 18:35:37 EEST, Emmanuele Bassi @.***> wrote:

Usually all you need is use gtk::subclass::prelude::*;

This is only even remotely true if all you're using is GTK.

Once you start using libadwaita and gstreamer—in other words, once you start writing a useful application instead of an example—you get really deep into the woods

Can you provide an example where this fails somehow?

There's a lot of code out there importing the preludes of gtk, gstreamer and adwaita in the same module. This generally works fine.

This only causes problems if they're using different versions of glib. But then you'll run into other problems sooner or later anyway.

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

sdroege avatar Oct 11 '22 07:10 sdroege

[Side note: we ended up discussing about this on Matrix]

The main issue is exactly the case where the prelude ends up loading different versions of the dependencies. Yes: bad things may happen anyway down the line, but the whole thing is quite unergonomic and hard to debug. It's also a side effect of using one module from Git and an unrelated module from a stable release—for instance, using libadwaita from Git and GStreamer from a stable tag. Nominally, those two are completely different leaves of the dependency tree, but of course they end up interacting. Forcing to switch everything to Git or to stable releases is quite annoying because it removes a lot of the granularity nominally available.

Of course, I don't really have any real solution, but it would be nice to have a better error message instead of a dump of weird conflicts.

ebassi avatar Oct 12 '22 09:10 ebassi

What's the confusing error you mean? X does not implement IsA<Object> or expected X but got X or similar?

sdroege avatar Oct 12 '22 09:10 sdroege

Something to that effect, yes. It's been more than 6 months, and I was able to figure out what the issue was.

At the very least, I'd be happy with a FAQ somewhere, explaining that if you mix dependencies and they bring their own versions of the dependencies, then you might get this kind of errors during compilation; the solution is to either use Git for every dependency involved with glib-rs/gtk-rs, or to use stable releases everywhere.

ebassi avatar Oct 12 '22 09:10 ebassi

At the very least, I'd be happy with a FAQ somewhere, explaining that if you mix dependencies and they bring their own versions of the dependencies, then you might get this kind of errors during compilation; the solution is to either use Git for every dependency involved with glib-rs/gtk-rs, or to use stable releases everywhere.

Sounds like a good issue for: https://github.com/gtk-rs/gtk-rs.github.io/issues

Hofer-Julian avatar Oct 12 '22 10:10 Hofer-Julian