Multiple Sources (Requires #143)

[Lucki]

Multiple sources are now possible and randomly selected. A hidden feature is that the list gets sorted by source type on opening the preferences window. They're stored each in its own gsettings. Timestamps as IDs remembered for mapping in /sources, general settings in /sources/general/ and specific settings in the respective source subfolders /sources/${Source}/.

I'm not sure if I introduced the bug through the rework above, but the timer wasn't starting again after closing the preferences window. This was fixed in e7aa0c4. I'm experiencing that the timer resets after some time. It seems the cause is that the extension runs in two contexts which can't be caught by the instance/singleton workaround. This might be the same issue behind #135 - there are basically multiple timers and observers to settings changes running in the background.

Screenshots

 

This will fix #102 fix #91 fix #57 fix #50 fix #39 This extension works for Gnome 42+ so this will fix #129 fix #133 fix #138

[Lucki]

@ifl0w Any opinions?

[pascallj]

Hi @Lucki, I am watching this repository so I have seen your commits rolling in in the past few weeks. It seems like you put an extraordinary amount of effort in it to make this extension even better. Haven't reviewed the code but at least a big :+1: for your effort! However if you ask me, this PR has gotten so big it is impossible to review it. Especially since @ifl0w has said before his time is limited at the moment and this project hasn't his primary focus.

Best practice is to split each new feature/fix into different PR's and only combine related commits in one PR. That way it is manageable and you can see what changes are required for which feature or fix and review it accordingly.

Just some general advice or at least my personal opinion.

[Lucki]

I initially wanted to do it with the splitting, but the problem is that everything builds upon the new UI. (And I really don't want to build that again in XML)

I may try to combine similar themed commits where it makes sense.

Edit: I've tried and after fighting with git I'm not sure anymore all changes are correctly carried over. I'll leave it as it is for now.

[pascallj]

I see. Maybe I am too picky regarding these practices 😉.

It should be possible to update your commits with a git rebase and force push, but it can be quite tricky indeed.

[ifl0w]

Wow! :rocket: ... That is a respectable amount of work you have put into improving the extension @Lucki! Thanks for all your effort, and I'll try my best helping to merge and release your work! :smiley: As @pascallj mentioned, I am still pretty limited time-wise until early 2023. So I might still take a while to review all of this.

Many of your changes have been on my to-do list for ages and should be integrated anyways. Squashing and reordering some commits into more digestible pieces and merging those first would be great, but I'll need to read through all changes to see if this is easily possible. If you want to build on the changes in this PR, then you should definitely open new PRs that target this PR as the base, as suggested by @pascallj. This will make it easier to review any additional changes.

Again, thank you very much for your contribution! This is amazing work!

[Lucki]

I've tried and failed the last time, I could try again to untangle some of the commits, but now there are even more of them :D

A bit hidden at GitHub is that you can review commits instead of everything at once if that's helpful. Click on the first commit, and you can follow my changes. My first post has all edits in the same order. But I admit that it would be utterly difficult because I probably changed the same thing later on again.

[ifl0w]

@Lucki Cleaning up the commit history can be done in the end right before merging if necessary. So you don't need to think about that too much now.

The most important part is getting your work merged ASAP. Having one large PR will slow us down in addressing all comments and discussions and will probably take way longer to merge than dealing with smaller PRs. :) So I'd suggest splitting this PR into three smaller PRs (you can also split into more PRs if you want):

1. I'd suggest that the first part should be the initial rewrite using blueprints up to (including) commit 8e5d8a6
2. The second part could be all changes building on the blueprint rewrite up to (including) commit 535d0a6
3. The third piece would be everything after 535d0a6 (i.e., the switch to TS)

Especially the addition of the Github Workflow and the switch to TS is great work, but I think I'll need to look at it more closely. There are two issues I see with these changes:

1. The files essentially have been renamed (.js -> .ts), but the changes are not tracked as renaming in Git, and thus changes inside the files are not transparent. It should be possible to maintain the git history in the files.
2. I don't love to see the Adwaita headers and the libadwaita binary tracked in the repository. I think we should try to avoid this.

You don't need to mess around with Git much for splitting the PR. just check out the respective commit and push them to a new branch and open a PR from that branch. Just target the branch of the PR that has to be merged first.

Let me know if you need any help with that or have better suggestions. :)

[Lucki]

You don't need to mess around with Git much for splitting the PR. just check out the respective commit and push them to a new branch and open a PR from that branch. Just target the branch of the PR that has to be merged first.

I hope I've done that right now. I can't target my own branch because then my PR would be opened in my own repo.

The PRs are chained now and hopefully resolve the later ones correctly when the first ones are merged:

143 → #141 → #144 → #145 → #146 → #147 → #148 → #149 → #150 → #151 → #152 → #153 → TS (Todo)

[ifl0w]

@Lucki You have split it in more PRs than I expected, but that's fine for me. :) Thanks for your hard work!

[Lucki]

The first thing I noticed is the id that is passed to the adapters and integrated into a string and then forwarded to getSettings as the second argument, but getSettings has no second parameter. What is that used for?

I need a path to support relocatable schemas. It's described here a bit further down: https://blog.gtk.org/2017/05/01/first-steps-with-gsettings/

If your application uses accounts, you may want to look at relocatable schemas. A relocatable schema is what you need when you need multiple instances of the same configuration, stored separately. A typical example for this is accounts: your application allows to create more than one, and each of them has the same kind of configuration information associated with it.

GSettings handles this by omitting the path in the schema […]

Instead, we need to specify a path when we create the GSettings object […]

And the ID is needed for:

It is up to you to come up with a schema to map your accounts to unique paths.

It's possible that my untangling shoved this into the next PR. I'll look it up.

It was in the convenience.js which is now deleted, because of the rebase this part is missing until I also deleted it later on. It seems the first mention of it is now in the move to TS e46b119 (#154) and rebuilds parts of the obsolete convenience.js and the replacement extensionUtils.js because the need of specifying a path.

class Settings {
    private _settings: Gio.Settings;

    constructor(schemaId?: string, schemaPath?: string) {
        if (schemaPath === undefined) {
            this._settings = ExtensionUtils.getSettings(schemaId);
        } else {
            // We can't give a path so we need to rebuild the function:
            const schemaObj = this._getSchema(schemaId);

            // Everything above for… this…
            this._settings = new Gio.Settings({settings_schema: schemaObj, path: schemaPath});
        }
    }

    private _getSchema(schemaId?: string) {
        // https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/gnome-43/js/misc/extensionUtils.js#L211
        if (!schemaId)
            schemaId = Self.metadata['settings-schema'];

        // Expect USER extensions to have a schemas/ subfolder, otherwise assume a
        // SYSTEM extension that has been installed in the same prefix as the shell
        const schemaDir = Self.dir.get_child('schemas');
        let schemaSource;
        const schemaPath = schemaDir.get_path();
        if (schemaDir.query_exists(null) && schemaPath !== null) {
            schemaSource = Gio.SettingsSchemaSource.new_from_directory(schemaPath,
                Gio.SettingsSchemaSource.get_default(),
                false);
        } else {
            schemaSource = Gio.SettingsSchemaSource.get_default();
        }

        const schemaObj = schemaSource?.lookup(schemaId, true);
        if (!schemaObj)
            throw new Error(`Schema ${schemaId} could not be found for extension ${Self.metadata.uuid}. Please check your installation`);

        return schemaObj;
    }

// […]

---

The second thing is that the convenience.js file is used, which is not part of the repository anymore. The dependency should be removed if possible.

At the time I wrote this it was still part of this repository. I figured this file unneseccary too, the removal is already included in one of the future PRs.

[ifl0w]

Ok, I will look into squashing the commits in the end to capture all changes that belong together in a single commit. Obv. this would be a huge commit, and I'll investigate if that's a good idea or not in the end.

Thanks again for the clarifications.