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

Use TypeScript (Requires #153) #154

Merged
merged 87 commits into from
Aug 15, 2023
Merged

Use TypeScript (Requires #153) #154

merged 87 commits into from
Aug 15, 2023

Conversation

Lucki
Copy link
Contributor

@Lucki Lucki commented Dec 12, 2022

Moved everything to TypeScript which will generate to JavaScript. This allows proper tooling like syntax checks, code highlights and docs additionally to type safety. It actually found quite a lot of smaller issues in the current code which are now fixed.
Because of this it's now required to have npm installed for building. I added a GitHub actions workflow which adds a build artifact to each push so easy access to nightly builds without manual building is available: example
One smaller caveat for TypeScript currently is that it can't properly type classes based on GObject.registerClass(). From what I've read it's currently an ongoing process to migrate this to proper ES6 classes in the GJS team.

Additionally, adapter can now request multiple new images at once and a lot of smaller fixes.


This already resolves comments from #141 :

  • Libadwaita is now downloaded on demand
  • I tried to preserve the JS history by moving the files.

Fun fact: While moving these commits from the other PR the majority of d6b5e6a was lost. I don't know why, I don't know how, but I had to rewrite almost everything.

Found the lost commit and inserted at the right place - I hope no other things were lost in the process…

@ArjixWasTaken
Copy link

Nice PR, don't mind if I fork it :^)

@ifl0w
Copy link
Owner

ifl0w commented Apr 6, 2023

@Lucki finally, at the last stage :D ... Please rebase; I'll try to find the time to review during the next week!

Lucki added 22 commits April 7, 2023 12:50
… since blueprint-compiler building had to be done anyway.
* allow requesting multiple images
* timer fixes
* hover preview fixes

Don't fetch more images than needed:
Before this always the amount of displays were requested even if
not used later on.
…for real
Older history entries from gsettings might not yet have a adapter
associated.
This properly types this and fixes related null checks missing.
Introduces new function to resolve a new random path according to an
already resolved path with similar elements.

Reference:
ifl0w#143 (comment)
* Prefix HOME variable to correctly get the users home path
* Prevent whitespace issues by quoting the users home path variable

Reference: ifl0w#143 (comment)
…and assume soup figures out throttling on its own
to not overload servers.
…replacing the fixed resolution with ratios and minimal resolution
@Lucki
Copy link
Contributor Author

Lucki commented Jul 10, 2023

The fix appeared in https://gitlab.gnome.org/GNOME/gnome-shell/-/tags/45.alpha (!2781) so removing the workaround will bump the minimum required version to Gnome 45 if it won't get backported later on.

Copy link
Owner

@ifl0w ifl0w left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Please fix the one commit message, and then let's merge it.

package.json Outdated
Comment on lines 3 to 12
"@gi-types/adw1": "latest",
"@gi-types/base-types": "latest",
"@gi-types/gjs-environment": "latest",
"@gi-types/gtk4-types": "latest",
"@gi-types/shell": "latest",
"@typescript-eslint/eslint-plugin": "latest",
"@typescript-eslint/parser": "latest",
"eslint": "latest",
"eslint-plugin-jsdoc": "latest",
"typescript": "latest"
Copy link
Owner

Choose a reason for hiding this comment

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

Honestly, I am not sure if it is a good idea to use "latest" here for all dependencies.&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pinned them to the current version

src/utils.ts Outdated
@@ -169,8 +169,6 @@ function getMonitorCount(): number {
// @ts-expect-error
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
const currentDisplay = global?.display as Meta.Display;

// eslint-disable-next-line @typescript-eslint/no-unsafe-call
Copy link
Owner

Choose a reason for hiding this comment

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

Fyi, this kind of commits can easily be avoided by using git commit --amend or alternatively git commit --fixup followed by a git rebase --autosquash <ref>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I even called it fixup but already pushed and did not want to force push while you're possibly in a review state.

src/adapter/baseAdapter.ts Outdated Show resolved Hide resolved
Comment on lines 349 to 382
if (type === 0 || type === 2) {
// FIXME: These are currently hardcoded for the few available wallpaperManager
if (wallpaperUri.includes('merged_wallpaper') || wallpaperUri.includes('cli-a') || wallpaperUri.includes('cli-b'))
// merged wallpapers need mode "spanned"
backgroundSettings.setString('picture-options', 'spanned');
else
// single wallpapers need mode "zoom"
backgroundSettings.setString('picture-options', 'zoom');

Utils.setPictureUriOfSettingsObject(backgroundSettings, wallpaperUri);
}

if (type === 1) {
// FIXME: These are currently hardcoded for the few available wallpaperManager
if (wallpaperUri.includes('merged_wallpaper') || wallpaperUri.includes('cli-a') || wallpaperUri.includes('cli-b'))
// merged wallpapers need mode "spanned"
screensaverSettings.setString('picture-options', 'spanned');
else
// single wallpapers need mode "zoom"
screensaverSettings.setString('picture-options', 'zoom');

Utils.setPictureUriOfSettingsObject(screensaverSettings, wallpaperUri);
}

if (type === 2) {
// FIXME: These are currently hardcoded for the few available wallpaperManager
if (wallpaperUri.includes('merged_wallpaper') || wallpaperUri.includes('cli-a') || wallpaperUri.includes('cli-b'))
// merged wallpapers need mode "spanned"
screensaverSettings.setString('picture-options', 'spanned');
else
// single wallpapers need mode "zoom"
screensaverSettings.setString('picture-options', 'zoom');

Utils.setPictureUriOfSettingsObject(screensaverSettings, backgroundSettings.getString('picture-uri'));
Copy link
Owner

Choose a reason for hiding this comment

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

I honestly never saw that showing up anywhere. But the gnome settings app also sets the lockscreen value on wallpaper change so it should show up somewhere?

I don't know if it is actually used anywhere in gnome anymore. It used to show up when locking/unlocking your screen iirc. Now there is always the blurred background I think... 🤷

For some reason Gdk was now unable to get my monitor count.
This stops spoiling the API key in sidecar files when saving an image.
When pausing the timer, locking the screen and unlocking again,
I got two new wallpaper in a short amount of time the next time
the timer runs. The observer were still active and unpausing
activated all still observing timer.
@ifl0w ifl0w merged commit 15df01b into ifl0w:WIP_v3.0.0 Aug 15, 2023
2 checks passed
ifl0w pushed a commit that referenced this pull request Aug 15, 2023
ifl0w pushed a commit that referenced this pull request Feb 11, 2024
ifl0w pushed a commit that referenced this pull request Feb 11, 2024
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