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

Port libsoup3 #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Port libsoup3 #52

wants to merge 2 commits into from

Conversation

prahal
Copy link

@prahal prahal commented Oct 10, 2022

I am currently on gnome shell 43.0 binded to libsoup3 gir on Debian and it works.
It also works on a Debian install with gnome-shell 42.4 binded to libsoup 2.4.

But there are blind spots in the error paths:

  • in:
body = this.httpSession.send_and_read_finish(result, err);
                    if (!body) {
                        error('Body is null: ' + err);

I do not know if the err (a GError) is converted to a string properly on error, I guessed.
Also, the 'Body is null' prefix might not be the best.

  • I converted blindly:
    error('result error: ' + JSON.stringify(response));
    to
    error('result error: ' + JSON.stringify(result));
    I do not know what a GAsyncResult stringified will become. The issue is I do not know how it worked before with a SoupMessage.

So it is in a "works for me" form. As those changes are specific to the libsoup3 code paths maybe it could be merged and those issues handled when those code paths are triggered.

@prahal prahal force-pushed the port-libsoup3 branch 4 times, most recently from f8a3343 to a739376 Compare October 11, 2022 22:59
@prahal
Copy link
Author

prahal commented Oct 11, 2022

Both previous issues are fixed.
The part about the body is obsolete (I found that gerror are exceptions in javascript, which is at the same time obvious and nowhere documented).
And I replaced the JSON.stringify of the result object with two stringify: one of response and the other of the body (that was previously a subfield of response).

@dveeden
Copy link

dveeden commented Oct 27, 2022

This would fix #51 right?

@prahal
Copy link
Author

prahal commented Oct 29, 2022

@dveeden yes it fixes #51 thus why there a subcommit to add gnome shell 43 compatibility. Cheers

@thomasnemer
Copy link

Would love to see this PR merged :)
Thanks @prahal

@Shelvak
Copy link
Contributor

Shelvak commented Nov 28, 2022

Working in Gnome43 (manjaro version) @alexduf could we have this merged please?
EDIT: And thanks @prahal to get this working =D

@KSXGitHub
Copy link

KSXGitHub commented Dec 7, 2022

I have been using this as a PKGBUILD for 11 days now, it's has been working flawlessly. I would love to see this merged.

@machitgarha
Copy link
Contributor

The only issue is that, if the notifications cannot be grabbed (e.g. the token being invalid, expired or missing, or the device being offline), notifications count will be shown as "0" instead of "!" (i.e. the previous behavior).

BTW, good job! Thanks!

@machitgarha
Copy link
Contributor

There's also another issue happening here. The notifications count doesn't update at the interval and regularly (e.g. every one minute), but only when I reload the extension or make a login (e.g. after locking the screen). Kinda weird.

@Shelvak
Copy link
Contributor

Shelvak commented Dec 31, 2022

There's also another issue happening here. The notifications count doesn't update at the interval and regularly (e.g. every one minute), but only when I reload the extension or make a login (e.g. after locking the screen). Kinda weird.

I noticed the same, many times it took a lot to update (0 => N or N => 0...) I didn't dig into it yet..

@username0136
Copy link

username0136 commented Mar 7, 2023

@prahal It appears that the current maintainer is no longer active. I believe it would be beneficial to maintain the forked version. Would you be interested in taking on this responsibility?

@prahal
Copy link
Author

prahal commented Mar 8, 2023

@git-Hmmm I cannot spend the time to maintain a proper fork (published on extensions.gnome.org). Mainly because currently I invest time in a few other projects. But I could maintain an initial code fork on GitHub with an issue tracker. Maybe later on I could publish the extension with proper metadata for a fork on extensions.gnome.org.

@prahal
Copy link
Author

prahal commented Mar 8, 2023

@git-Hmmm also note that a maintained fork already exists. https://github.com/mackdk/gnome-github-manager . Maybe it would be better to check if it supports libsoup3 and else port my patch to it than starting a new fork.

@username0136
Copy link

@prahal Thanks for the info. I will checkout this fork.

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.

7 participants