GNOME Bugzilla – Bug 782356
Split Monitor and MonitorConstraint out of layout.js
Last modified: 2021-07-05 14:32:50 UTC
While debugging an error today in our (Endless) downstream version of the shell (together with Philip Chimento, thanks!), I hit a problem that seemed to have been cause by a circular import kind of situation, provoked by a new class we introduced, that depends on modalDialog.js and got instanced from windowManager.js: ForceAppExitDialog. The problem was that GNOME Shell would fail to initialize that ForceAppExitDialog class because, apparently, the constructor of the parent class (ModalDialog) was not being run, but the one from Base, throwing an error like this one: (gnome-shell:7555): Gjs-WARNING **: JS ERROR: TypeError: this.contentLayout is undefined ForceAppExitDialog<._init@resource:///org/gnome/shell/ui/forceAppExitDialog.js:57:9 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:13 _Base.prototype._construct@resource:///org/gnome/gjs/modules/lang.js:110:5 Class.prototype._construct/newClassConstructor@resource:///org/gnome/gjs/modules/lang.js:213:13 WindowManager<._showForceAppExitDialog@resource:///org/gnome/shell/ui/windowManager.js:1847:13 wrapper@resource:///org/gnome/gjs/modules/lang.js:178:13 After debugging this with Philip, we found out the problem: Since modalDialog.js imports layout.js, which ends up getting windowManager.js imported, which imports forceAppExitDialog.js... which imports modalDialog.js again, at the time that the ForceAppExitDialog class is being created its super class is not completely defined yet, causing the constructor of the ModalDialog not to be called and therefore the whole thing to fail. The obscure side of things here is that the version of GJS I was using (1.47.3 for now) was not handling that as the newer version (1.48) will do, so no syntax error was thrown at all because of this circular import, making the problem quite sneaky. Following Philip's advice, I experimented with taking Monitor and MonitorConstraint classes out of layout.js into a new module monitor.js, so that I could include that instead of the whole layout.js module from many places where that's all it's needed, with the aim to reduce the chances to hit this problem... and that worked nicely for us: I can now instantiate our ForceAppExitDialog class with no problems from windowManager.js. So, while I realize this is a problem we hit downstream because of our particular new class, I think it would probably be interesting to land it upstream too, since this should help avoiding this problem should a situation like this one materialized down the road here too, and well... also because honestly it seems that Monitor and MonitorConstraint are classes fairly orthogonal to the rest ot layout.js, so it probably won't hurt either.
Created attachment 351396 [details] [review] Split Monitor and MonitorConstraint out of layout.js These two classes are fairly independent from the rest of layout.js, and having them in there forces us to import the whole file in many places where only Monitor or MonitorConstraint is needed. So, let's move it to their own specific file so that we can avoid importing the whole of layout.js in those places. This should reduce the chances to hit circular imports if we, down the road, declare a class from some of the modules recursively imported from layout.js depending on modules that haven't been properly initialized yet, which would cause obscure errors in GJS < 1.48, and a syntax error in newer versions (which got stricter about const definitions).
Review of attachment 351396 [details] [review]: I see the point, but without any actual code in gnome-shell itself that requires this change, I don't see a strong enough reason to break existing code (any extension that uses MonitorConstraint)
Good point, I did not think about extensions. Btw, do we have an idea of which extensions could be using that? One thing we could do could be to include a "compatibility layer" for some releases not to break extensions that you would define inside layout.js, encouraging extensions maintainers to get updated anyway (e.g. in the release notes). What I'm thinking is maybe replacing, in layout.js, the following line I have now: const Monitor = imports.ui.monitor; ...with something like this: // These constants import the Monitor and MonitorConstraint classes // directly (instead of just importing the module) in order not to // break extensions currently relying on them being in layout.js. const Monitor = imports.ui.monitor.MonitorConstraint; const MonitorConstraint = imports.ui.monitor.MonitorConstraint; Then, in LayoutManager::_updateMonitors(), also inside layout.js, replace this: this.monitors.push(new Monitor.Monitor(i, screen.get_monitor_geometry(i))); with this: this.monitors.push(new Monitor(i, screen.get_monitor_geometry(i))); This way, any external thing relying on Layout.MonitorConstraint would still work, but internally we could get the shell to import monitor.js instead of layout.js in most places, as my patch currently does. What do you think?
(In reply to Mario Sánchez Prada from comment #3) > [...] > // These constants import the Monitor and MonitorConstraint classes > // directly (instead of just importing the module) in order not to > // break extensions currently relying on them being in layout.js. > const Monitor = imports.ui.monitor.MonitorConstraint; > const MonitorConstraint = imports.ui.monitor.MonitorConstraint; I've obviously meant this: const Monitor = imports.ui.monitor.Monitor; const MonitorConstraint = imports.ui.monitor.MonitorConstraint;
I don't know, "we may write code in the future that would introduce a circular dependency" still sounds like a very vague reason, and in particular splitting out Monitor from the only module that uses it looks odd. Unless we make this a general pattern and move *any* class to its own module, doing it just for those two appears rather random.
(In reply to Florian Müllner from comment #5) > I don't know, "we may write code in the future that would introduce a > circular dependency" still sounds like a very vague reason, and in > particular splitting out Monitor from the only module that uses it looks odd. Monitor is only used in layout.js, true, but MonitorConstraint is used from quite a few places that are importing the whole module when that class is the only thing they need. From the patch: js/gdm/loginDialog.js js/ui/legacyTray.js js/ui/messageTray.js js/ui/modalDialog.js js/ui/osdWindow.js js/ui/overview.js js/ui/padOsd.js js/ui/screenShield.js js/ui/unlockDialog.js The only reason I moved them out together is because they seemed related to me, but I wouldn't have any problem moving just MonitorConstraint into a monitorConstraint.js module, which would get imported from all those places. > Unless we make this a general pattern and move *any* class to its own module, I could actually argue this could be a good thing, but if you're worried about maintaining compatibility with extensions with this relatively simple split, doing such a change would be much much worse :-) Thus, I'd rather keep it simple and propose this change only for now, as that solves a real problem now already. > doing it just for those two appears rather random. I understand your point, but it looks random only from an upstream POV (which I realized is the main thing here of course), since you'll hit the same problem I hit as soon as you start adding code in any of those files if those additions create a cycle... and that bug is not a very obvious one (at least not to me). Granted, GJS 1.48 will make this a much more visible error, so perhaps it does not make sense to do the change just yet usptream (I still think it would make sense, though), but I was still hoping we could land this change (or something similar) so that we can keep our downstream fork as close to upstream as possible, which is usually good. This said, I was also assuming that this, while not being strictly needed, would not hurt either but that was when I did not consider extensions and didn't need to come up with a compat layer like the proposed above, so I'm not feeling like pushing much harder really. Please, let me know what you decide. If you are happy to take it, I can prepare the compat layer, otherwise I'll simply keep the split in our downstream fork for now.
Pinging again on this, any update on whether you'd be happy to take this patch, all things considered?. Thanks in advance
Not really - if we had an actual problem that was solved by moving code around, we could make a judgment call on where to put it. Right now all we have is hand-waving ...
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.