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

GNOME 45 port of Shell extension #1679

Merged
merged 11 commits into from
Sep 19, 2023

Conversation

retrixe
Copy link
Contributor

@retrixe retrixe commented Sep 17, 2023

  • Migrate code over to ESM.
  • Migrate prefs.js and extension.js to export needed objects.
  • Drop 'use strict' from ES Modules since it's unneeded as per JS spec.
  • Drop gettext and getIcon helpers on Extension.
  • Drop usages of ByteArray in favour of TextDecoder.
  • Drop var in favour of export let.
  • Remove __init__.js, move its setup code to a function in utils.js
  • Work around GNOME Shell not exporting some values to notification.js
  • Add workarounds for config.js/remote.js to be imported from ES modules
  • Use new Quick Settings API to add tile.
  • Update ESLint settings to support ES modules and newer JS features
  • Update _ornamentLabel name, fixes quick settings

Fixes #1665

- Migrate most code over to ESM.
- Migrate prefs.js and extension.js to export needed objects.
- Drop 'use strict' from ES Modules since it's unneeded as per JS spec.
- Drop gettext and getIcon helpers on Extension.
- Drop usages of ByteArray in favour of TextDecoder.
- Drop `var` in favour of `export let`.
- Add some FIXME comments.
Until the rest of the codebase is migrated to ESM, I don't see a better
way other than duplicating code into .mjs files which is not much better
@retrixe retrixe marked this pull request as draft September 17, 2023 13:38
@retrixe retrixe marked this pull request as ready for review September 17, 2023 18:00
Copy link
Collaborator

@andyholmes andyholmes left a comment

Choose a reason for hiding this comment

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

Nicely done!

I didn't look too deep, since there's just a lot of maintenance to do everywhere in GSCOnnect, so just a few tweaks and I think this will be good to merge.

However, the metadata.json will have to be updated to "45" as the shell-version.

src/extension.js Outdated Show resolved Hide resolved
src/extension.js Outdated Show resolved Hide resolved
@retrixe
Copy link
Contributor Author

retrixe commented Sep 17, 2023

It seems I'm experiencing a major issue on Fedora 39 + GNOME 45, where my device is seemingly unable to receive information from GSConnect despite being connected. I can see current info about the phone in my GSConnect menu and receive notifications and everything else, but I can't see laptop battery info, media players, receive files from laptop, etc from the phone. These did work fine with the stable GNOME 44 version (and Valent w/ GNOME 45 port works fine too).

I'm not sure what the root cause could be, seeing that the service and preferences are separate (and using the buttons there don't work either), and there are seemingly no relevant errors or stack traces in journalctl -xe or the generated support log.

@andyholmes
Copy link
Collaborator

It seems I'm experiencing a major issue on Fedora 39 + GNOME 45, where my device is seemingly unable to receive information from GSConnect despite being connected...

I would guess this is because GSConnect can now run on multiple ports, while an older instance may bind port 1716 and disallow re-use. Short answer: make sure you kill all existing instances of GSConnect and only the version you're working on is running.

@retrixe
Copy link
Contributor Author

retrixe commented Sep 17, 2023

This doesn't seem to be the case, I only have a single GSConnect daemon running, and the issue persists after a system restart as well :/

@andyholmes
Copy link
Collaborator

This doesn't seem to be the case, I only have a single GSConnect daemon running, and the issue persists after a system restart as well :/

Yeah, I can confirm this. I don't really know what's causing it, since backing up to main branch it seems to work fine. I guess the service may be importing something shared that's getting it confused?

@retrixe
Copy link
Contributor Author

retrixe commented Sep 18, 2023

It's not the .mjs files causing it, the imports are read correctly in the preferences and service from my logging, and the only imports that are shared appear to be those files, the rest are all isolated.

Nothing outside src/shell is importing from src/shell, same goes for extension.js and prefs.js (used regex imports\.(?!gi)(?!system)(?!searchPath)(?!config)(?!utils)(?!preferences)(?!byteArray)(?!format)(?!service)(?!tweener)(?!remote)(?!gettext)(?!wl_clipboard) to check)

Perhaps it's a change in gjs/gi libraries that affects something in the service, will try to look into it further :/

@andyholmes
Copy link
Collaborator

Perhaps it's a change in gjs/gi libraries that affects something in the service, will try to look into it further :/

I don't believe so, I just pulled from main and restarted the daemon and it worked fine. Logs from kdeconnect-android may be more enlightening though.

@andyholmes
Copy link
Collaborator

andyholmes commented Sep 18, 2023

Actually, seems to work for me now. Is nothing being printed in the log that's any help? I assume you're running with debug enabled?

@retrixe
Copy link
Contributor Author

retrixe commented Sep 19, 2023

Well, this is really weird, I just restarted KDE Connect on my phone and the daemon works fine now lol
Switching the daemon off and then on also reconnects properly (although there is a slight delay before the phone starts responding/receives any requests to e.g. Ring in the first place), not sure how this bug came about in the first place, but I guess this has nothing to do with this PR after all.

FWIW yes, I was using debug and the only logs I saw were about sending/receiving data and few unrelated errors wrt SFTP. Not sure how to fetch logs from kdeconnect-android (adb logcat doesn't seem to have many relevant logs), but the bug isn't occurring anymore and I'm not able to repro it anymore

@andyholmes
Copy link
Collaborator

Okay, cool, is there anything left to resolve? I think this is good enough to get some testing. GNOME 45 is released in a few days, so it would good to get some broader testing.

@retrixe
Copy link
Contributor Author

retrixe commented Sep 19, 2023

Nope, everything is complete and ready to merge 👍

@andyholmes andyholmes merged commit eb4997a into GSConnect:main Sep 19, 2023
2 checks passed
@andyholmes
Copy link
Collaborator

Thanks so much for working through this!

@kloczek
Copy link

kloczek commented Oct 23, 2023

Is it anything else still on outstanding list which holds new version released with GNOME 45 support? 🤔

@andyholmes
Copy link
Collaborator

#1694 needs to be fixed, probably as a part of #1683, although it could just be patched.

@kloczek
Copy link

kloczek commented Oct 23, 2023

Thank you to let me know 👍

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.

GNOME 45 support
3 participants