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

refactor!: esm syntax for GNOME 45 #277

Merged
merged 47 commits into from
Sep 18, 2023
Merged

Conversation

bennypowers
Copy link
Contributor

@bennypowers bennypowers commented Sep 10, 2023

What I did

  • shuffle syntax to esm, per the migration guide.
  • replace _init calls with constructors
  • move ./widgets.js to ./preferences/widgets.js
    • modules in the prefs.js module graph have different semantics so it's probably smart to keep 'em separated
  • refactor ./css/index.js to es2015 and add it to prettier
  • refactor some of the gettext code to suit the new hotness: we have to instantiate anything relying on metadata (like some of the message) in the Extension and ExtensionPreferences constructors now.
  • refactor the project structure such that the only modules in the project root are extention.js and prefs.js
  • refactor logger.js to use console.* methods
  • broke ThemeManager down into subclasses for prefs and extension, rather than passing in a flag

What I'm not sure about

  • the Session Modes docs haven't been updated yet so i'm pretty sure what i did here wrt session modes is broken
  • i think that in addition to refactoring logger to use console methods, it should be made a base class or class mixin, since settings is now available on BaseExtension#getSettings()

jmmaranan and others added 7 commits August 13, 2023 11:52
This initial commit shuffles the syntax to esm, per the
[migration guide][guide].

It's very likely that some of the imports are incorrect, specifically
those that require different URLs when imported from prefs.js. There are
likely other bugs as well, so this should be considered a draft

[guide]: https://gjs.guide/extensions/upgrading/gnome-shell-45.html#esm
also moves widgets module to ./preferences
@bennypowers bennypowers changed the base branch from gnome-45-esm to main September 10, 2023 13:08
@bennypowers
Copy link
Contributor Author

bennypowers commented Sep 11, 2023

**SOLVED**: prefs now launches

So far I get this when trying to launch prefs:

The settings of extension forge@jmmaranan.com had an error:

Error: Tried to construct an object without a GType

Stack trace:

PreferencesPage@file:///home/bennypowers/.local/share/gnome-shell/extensions/forge@jmmaranan.com/lib/prefs/widgets.js:15:8
SettingsPage@file:///home/bennypowers/.local/share/gnome-shell/extensions/forge@jmmaranan.com/lib/prefs/settings.js:13:5
fillPreferencesWindow@file:///home/bennypowers/.local/share/gnome-shell/extensions/forge@jmmaranan.com/prefs.js:48:16
_loadPrefs@resource:///org/gnome/Shell/Extensions/js/extensionPrefsDialog.js:36:18
async*_init@resource:///org/gnome/Shell/Extensions/js/extensionPrefsDialog.js:21:14
ExtensionPrefsDialog@resource:///org/gnome/Shell/Extensions/js/extensionPrefsDialog.js:12:4
OpenExtensionPrefsAsync@resource:///org/gnome/Shell/Extensions/js/extensionsService.js:139:33
async*LaunchExtensionPrefsAsync@resource:///org/gnome/Shell/Extensions/js/extensionsService.js:126:14
_handleMethodCall@resource:///org/gnome/gjs/modules/core/overrides/Gio.js:373:35
_wrapJSObject/<@resource:///org/gnome/gjs/modules/core/overrides/Gio.js:408:34
_init/GLib.MainLoop.prototype.runAsync/</<@resource:///org/gnome/gjs/modules/core/overrides/GLib.js:266:34

@bennypowers
Copy link
Contributor Author

I'm basically stuck on the error in the previous comment.

@jmmaranan can you reproduce? I observed this in GNOME OS nightly

@bennypowers
Copy link
Contributor Author

@fmuellner very kindly solved the previous error. Thank you!

next:

Requiring Gdk.js, version none: Typelib file for namespace 'Gdk.js' (any version) not found

most probably needs a version query param on an import

@bennypowers
Copy link
Contributor Author

next:

JS ERROR: Extension forge@jmmaranan.com: Error: Tried to construct an object without a GType
ThemeManagerBase@file:///home/bennypowers/.local/share/gnome-shell/extensions/forge@jmmaranan.com/lib/shared/theme.js:34:5
ExtensionThemeManager@file:///home/bennypowers/.local/share/gnome-shell/extensions/forge@jmmaranan.com/lib/extension/extension-theme-manager.js:12:5
@file:///home/bennypowers/.local/share/gnome-shell/extensions/forge@jmmaranan.com/extension.js:41:11
ForgeExtension@file:///home/bennypowers/.local/share/gnome-shell/extensions/forge@jmmaranan.com/extension.js:34:16
_callExtensionInit@resource:///org/gnome/shell/ui/extensionSystem.js:537:17
async*loadExtension@resource:///org/gnome/shell/ui/extensionSystem.js:450:33
_loadExtensions@resource:///org/gnome/shell/ui/extensionSystem.js:724:24
async*_enableAllExtensions@resource:///org/gnome/shell/ui/extensionSystem.js:730:48
_sessionUpdated@resource:///org/gnome/shell/ui/extensionSystem.js:765:20
async*init@resource:///org/gnome/shell/ui/extensionSystem.js:69:14
_initializeUI@resource:///org/gnome/shell/ui/main.js:290:22
start@resource:///org/gnome/shell/ui/main.js:163:11
@resource:///org/gnome/shell/ui/init.js:11:47
@resource:///org/gnome/shell/ui/init.js:21:20

@bennypowers
Copy link
Contributor Author

It runs!
image
still broken: launching settings from quick settings, and i don't think it's supposed to show both panel and quick settings... but it runs!

@bennypowers
Copy link
Contributor Author

OK I believe this is ready to go

@jmmaranan
Copy link
Collaborator

Thank you so much for this! I am currently rebasing to Fedora 39

@bennypowers
Copy link
Contributor Author

@jmmaranan please carefully review 1575157, I'm not so familiar with that feature

@jmmaranan
Copy link
Collaborator

I was not able to rebase to FC39 last time (I should never have installed any other packages). So I'll just download an ISO into Boxes.

For that commit, it is used to reposition the mouse during key navigation. But it has not worked since they started removing some API methods. I think starting from GNOME 40

@bennypowers
Copy link
Contributor Author

According to @fmuellner, in Extensions chat:

... gdkDisplay is always null, so you won't do anything
(and if it didn't return null, you'd run into the issue of trying to use gdk3 with gdk4)

So this might actually be an improvement, if it works 🤔

Copy link
Collaborator

@jmmaranan jmmaranan left a comment

Choose a reason for hiding this comment

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

@bennypowers - the po/forge.pot is not being produced after the changes to the Makefile. Please point the inputfiles for the xgettext command from ./extensions.js to each js file from lib/prefs/. xgettext is expecting files that contain _("translation string"), extensions.js does not have those.

@jmmaranan
Copy link
Collaborator

More comments:

  • The icons on preferences are broken.
  • Not part of ESM changes but can the dropdown toggles on preferences be fixed? Changing the toggle does not persist or nor apply the changes. It was broken last time after moving to libadwaita widgets.

image

@bennypowers
Copy link
Contributor Author

I believe translations and icons are fixed. can we defer the dropdown bug to a separate pr?

@jmmaranan
Copy link
Collaborator

Looks good to me

@jmmaranan jmmaranan self-requested a review September 18, 2023 09:53
@jmmaranan jmmaranan merged commit 6b354ba into forge-ext:main Sep 18, 2023
@bennypowers bennypowers deleted the gnome-45-esm branch September 18, 2023 10:00
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.

None yet

2 participants