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 some types to GNOME 46 + some additional types #23

Merged
merged 10 commits into from
Mar 28, 2024

Conversation

Totto16
Copy link
Collaborator

@Totto16 Totto16 commented Mar 27, 2024

This ports some types to GNOME 46 and adds a new file:
ui/lightbox

This fixes only types, I need for an extension, and some things that changed, that I saw while doing that.

@Totto16
Copy link
Collaborator Author

Totto16 commented Mar 27, 2024

Same as #22 these are just the Basics, and I have some complex types that are not correct yet, but those need some deep investigation, from where they come and how to fix them, and I am yet unsure, if they are from gnome shell or from other packages, @JumpLink you mentioned, that the generated types might be incorrect, or do I remember incorrectly?

Anyways, I'll look into these, that might take some time

@Totto16 Totto16 marked this pull request as draft March 27, 2024 15:47
@JumpLink
Copy link
Collaborator

JumpLink commented Mar 27, 2024

@Totto16 yes, the existing types might not be correct. So if you think there might be something wrong, go ahead and change it

@JumpLink
Copy link
Collaborator

JumpLink commented Mar 27, 2024

@Totto16 , @swsnr It would also be okay for me to divide the PR's into small parts which we then merge to collect feedback

@swsnr swsnr mentioned this pull request Mar 27, 2024
@swsnr
Copy link
Collaborator

swsnr commented Mar 28, 2024

@Totto16 In my MRs I marked every type which I added, update, or scrutinized for GNOME 46 with a /** @version 46 */ comment to keep track which types are already working with GNOME 46. Would you do the same in this PR?

I'm asking because I'm likely also going to touch the dialog modules when going through my extensions, so it'd help to see what's already been done.

@Totto16
Copy link
Collaborator Author

Totto16 commented Mar 28, 2024

@Totto16 In my MRs I marked every type which I added, update, or scrutinized for GNOME 46 with a /** @version 46 */ comment to keep track which types are already working with GNOME 46. Would you do the same in this PR?

I'm asking because I'm likely also going to touch the dialog modules when going through my extensions, so it'd help to see what's already been done.

Yeah, that's a good idea, I am going to do this too 👍🏼

@Totto16 Totto16 marked this pull request as ready for review March 28, 2024 16:00
@Totto16
Copy link
Collaborator Author

Totto16 commented Mar 28, 2024

Edit: this is redundant, I removed it 👍🏼

This is ready now, I added something, that might be controversial, I can remove it, if you don't like it:

here

it says The format() function (see [printf](https://wikipedia.org/wiki/Printf_format_string)) is available for all strings, but should only be used with gettext functions. In all other cases you should use JavaScript's [Template Literals](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Template_literals).

So I thought, If it is a good idea, to type the returns of the gettext() etc. functions explicitly with that types, so you don't have to import the global types, and so you don't have the risk of using it anywhere else. I prefer this, since it enforces this on a type basis. But if you don't prefer this, I can remove that final commit from this PR, since importing the global types isn't that "bad" and this is just a proposal from my side...

Anyway, let me hear your opinion on that ❤️

public open(timestamp: number, onPrimary: boolean): boolean;
public close(timestamp: number): boolean;
public open(): boolean;
//note: upstream has timestamp as paramater here, but it only will be used to call popModal, which doesn't accept that paramater anymore, so this is just a bug upstream
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this issue reported to upstream and being fixed?

If so, why not add a link here? If not, perhaps report it, to clarify if it's a bug here or with popModal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I intend to do that, but not yet 😓

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The bug is clearly here, since the last commits seem to get rid of the timestamp parameter everywhere else, they just missed it here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

made a MR on this issue: here

packages/gnome-shell/src/extensions/sharedInternals.d.ts Outdated Show resolved Hide resolved
@JumpLink JumpLink merged commit 311b6f6 into gjsify:main Mar 28, 2024
2 checks passed
@Totto16 Totto16 deleted the fixes branch March 28, 2024 17:47
@JumpLink
Copy link
Collaborator

@Totto16 Thank you very much! Would you also be interested in becoming a maintainer? Then you could review and merge each other. It's a bit inconvenient that I'm not currently developing an extension myself 😅

@Totto16
Copy link
Collaborator Author

Totto16 commented Mar 28, 2024

@Totto16 Thank you very much! Would you also be interested in becoming a maintainer? Then you could review and merge each other. It's a bit inconvenient that I'm not currently developing an extension myself 😅

Yeah for sure, that helps a lot.
And yeah, I'd love to be a maintainer ❤️ 👍🏼
@swsnr already did a great review of the types, so that definitely helps, since he also works with the types and that helps a lot when revieweing

@JumpLink
Copy link
Collaborator

That makes me happy :) I have sent you an invite

@swsnr
Copy link
Collaborator

swsnr commented Mar 28, 2024

@Totto16 Thank you very much! Would you also be interested in becoming a maintainer? Then you could review and merge each other. It's a bit inconvenient that I'm not currently developing an extension myself 😅

@JumpLink Not sure if it was meant to be implied here, but just in case there's a misunderstanding I'd like to point out that I am not a collaborator here and do not have permission to merge things here 🙂

I was just reviewing these changes because I'm interested 🙂

@JumpLink
Copy link
Collaborator

JumpLink commented Mar 28, 2024

Sorry @swsnr😅 your nickname is similar to @schnz 🫣

But it's great that more and more people are taking part and making such good contributions. You are very welcome :)

@swsnr
Copy link
Collaborator

swsnr commented Mar 29, 2024

Sorry @swsnr😅 your nickname is similar to @schnz 🫣

No worries 🙂

But it's great that more and more people are taking part and making such good contributions. You are very welcome :)

If it helps feel free to add me too 🙂

@JumpLink
Copy link
Collaborator

If it helps feel free to add me too

Send you an invite 📮

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.

3 participants