Implement manifest selection #71

SeaDve · 2022-02-15T14:23:04Z

Closes #17 and #8

TODO:

cleaner implementation and cleanup
handle new modules addition changes on manifest
watch manifest for on disk changes
status bar item for current active manifest

bilelmoussaoui · 2022-02-15T14:36:34Z

src/flatpakManifest.ts

@@ -467,6 +468,12 @@ export class FlatpakManifest {
        return new Command('flatpak', args, this.workspace)
    }

+    async deleteBuildDir(): Promise<void> {


Do we want to do that?

we call it on manifest changed and clean command

and it complains when tried to build init when .flatpak exists. I haven't tested much the edge cases yet, maybe tomorrow

hmm maybe just the .flatpak/repo. Since deleting .flatpak deletes every cached dependencies.

Edit: Changed to just delete the repoDir

SeaDve · 2022-02-16T03:39:28Z

Most of the things are now implemented and probably need a commit squash.

One thing I'm not sure about is whether the status bar item should open the active manifest or activate the select-manifest command. Currently, it activates select-manifest if there is no manifest found; otherwise, it opens the active manifest file.

It also needs more testing for edge cases

src/flatpakManifestManager.ts

bilelmoussaoui

I did a quick review, I will get back to this later today

package.json

src/workspaceState.ts

src/flatpakRunnerStatusItem.ts

bilelmoussaoui · 2022-02-16T10:16:53Z

src/flatpakManifest.ts

+     * @param other the more updated FlatpakManifest to get new info from
+     * @returns whether to request for a rebuild
+     */
+    updateFieldsFrom(other: FlatpakManifest): boolean {


Do we need to update the fields here? normally we would have separate FlatpakManifest or not?

Either could work, but I think the intent is shown better and clearer this way.

I just noticed you are calling this when the manifest receives changes, I think the best way to handle this is to just the new modified manifest as the active one (if it has the same uri as the currently active one)

changed to that behavior, and since an activeManifestChanged triggers to clear repoDir. It will ask to initialize a build every manifest modification. So I guess we can remove that message and just handle the build initialization automatically on a build or rebuild command.

It will ask to initialize a build every manifest modification.

But normally we already know if the manifest uri changed, if not and that manifest state says it was initialized that should give us enough info to know whether we should show the dialog or not

Nvm, that was not the reason. === operator in js doesn't check by value, but by instance. So had to use util.isDeepStrictEqual, either way, I think it is still better to remove that message, since running any build command should initialize a build

Oops, sorry. I didn't notice your last message. Was it okay though to remove the message with this reason?

running any build command should initialize a build

running any build command should initialize a build

It should, but my intention with that dialog is that someone not familiar with Flatpak/VScode could get an idea how to start a build and get the application up and running easily

Should not be a bigger problem with #55. But that makes sense

README.md

src/extension.ts

bilelmoussaoui · 2022-02-18T08:56:22Z

src/extension.ts

+      await this.manifestManager.doWithActiveManifestElseDisplayErr((activeManifest) => {
+        const runtimeTerminal = window.createTerminal(activeManifest.runtimeTerminal())
+        this.extCtx.subscriptions.push(runtimeTerminal)
+        this.activeTerminals.push(runtimeTerminal)


What happens if the user closes the terminal, is it dropped from the activeTerminals list?

ah yeah, that seems problematic. It is only used though to kill terminals when manifest changes. I think we can remove that since we can't also kill terminals that are provided by TerminalProviders.

And I realized killing terminals unknowingly maybe unideal

src/extension.ts

bilelmoussaoui · 2022-02-18T09:07:20Z

Btw, I sent you an invite for having write access to the repository just in case :)

SeaDve · 2022-02-18T09:19:13Z

thanks :D

bilelmoussaoui · 2022-02-26T14:04:17Z

Could use a rebase

SeaDve · 2022-02-26T14:19:07Z

I'll squash every commits for easier rebase

SeaDve · 2022-02-26T14:54:10Z

It shows this message when there is some error on the manifest and the error should be gone when the manifest is updated

SeaDve · 2022-02-26T15:00:28Z

there seems no cleaner way to do it

bilelmoussaoui · 2022-02-26T18:16:03Z

It shows this message when there is some error on the manifest and the error should be gone when the manifest is updated

that looks good enough

src/flatpakManifestManager.ts

src/flatpakRunner.ts

src/extension.ts

bilelmoussaoui · 2022-02-26T19:00:29Z

src/extension.ts

+    await this.ensurePipelineState()
+  }
+
+  private async resetPipelineState(): Promise<void> {


Should probably be part of workspaceState and called reset :)

we don't want to reset activeManifestUri too. Additionally, workspaceState shouldn't know which keys are part of the pipeline state as most of the pipeline state stuffs are handled in the extension.ts

src/extension.ts

bilelmoussaoui · 2022-02-27T08:03:09Z

Let us land this and do potential bugfixes/cleanups in following merge requests, awesome work!

bilelmoussaoui reviewed Feb 15, 2022

View reviewed changes

SeaDve commented Feb 16, 2022

View reviewed changes

src/flatpakManifestManager.ts Outdated Show resolved Hide resolved

SeaDve changed the title ~~Draft: Implement manifest selection~~ Implement manifest selection Feb 16, 2022

bilelmoussaoui reviewed Feb 16, 2022

View reviewed changes

bilelmoussaoui reviewed Feb 18, 2022

View reviewed changes

SeaDve force-pushed the manifest-manager branch from fa0762c to 052b56d Compare February 18, 2022 09:42

bilelmoussaoui mentioned this pull request Feb 20, 2022

Pickier parser #77

Merged

SeaDve mentioned this pull request Feb 22, 2022

Disable building when there are some errors in the manifest #78

Closed

bilelmoussaoui mentioned this pull request Feb 25, 2022

Drop the preview flag #79

Closed

3 tasks

SeaDve force-pushed the manifest-manager branch 2 times, most recently from df10c4c to 525b8da Compare February 25, 2022 13:07

bilelmoussaoui mentioned this pull request Feb 26, 2022

Handle require-version #83

Merged

SeaDve force-pushed the manifest-manager branch from 525b8da to dba5353 Compare February 26, 2022 14:53

SeaDve force-pushed the manifest-manager branch from dba5353 to 4e02f31 Compare February 26, 2022 14:57

SeaDve force-pushed the manifest-manager branch 3 times, most recently from 04b4f14 to 29faecf Compare February 26, 2022 15:09

SeaDve mentioned this pull request Feb 26, 2022

Naming issues #84

Closed

bilelmoussaoui reviewed Feb 26, 2022

View reviewed changes

SeaDve force-pushed the manifest-manager branch 2 times, most recently from fb3473c to 74e16f9 Compare February 27, 2022 00:11

feat: Implement manifest selection

1f9de44

SeaDve force-pushed the manifest-manager branch from 74e16f9 to 1f9de44 Compare February 27, 2022 00:31

bilelmoussaoui merged commit 08e3a14 into bilelmoussaoui:master Feb 27, 2022

bilelmoussaoui mentioned this pull request Feb 27, 2022

Allow selection of manifests #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement manifest selection #71

Implement manifest selection #71

SeaDve commented Feb 15, 2022 •

edited

bilelmoussaoui Feb 15, 2022

SeaDve Feb 15, 2022

SeaDve Feb 15, 2022

SeaDve Feb 16, 2022 •

edited

SeaDve commented Feb 16, 2022 •

edited

bilelmoussaoui left a comment

bilelmoussaoui Feb 16, 2022

SeaDve Feb 16, 2022 •

edited

bilelmoussaoui Feb 16, 2022

SeaDve Feb 16, 2022

bilelmoussaoui Feb 16, 2022

SeaDve Feb 16, 2022

SeaDve Feb 16, 2022

bilelmoussaoui Feb 18, 2022

SeaDve Feb 18, 2022

bilelmoussaoui Feb 18, 2022

SeaDve Feb 18, 2022 •

edited

bilelmoussaoui commented Feb 18, 2022

SeaDve commented Feb 18, 2022

bilelmoussaoui commented Feb 26, 2022

SeaDve commented Feb 26, 2022

SeaDve commented Feb 26, 2022 •

edited

SeaDve commented Feb 26, 2022

bilelmoussaoui commented Feb 26, 2022

bilelmoussaoui Feb 26, 2022

SeaDve Feb 26, 2022 •

edited

bilelmoussaoui commented Feb 27, 2022

Implement manifest selection #71

Implement manifest selection #71

Conversation

SeaDve commented Feb 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeaDve Feb 16, 2022 • edited

Choose a reason for hiding this comment

SeaDve commented Feb 16, 2022 • edited

bilelmoussaoui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeaDve Feb 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeaDve Feb 18, 2022 • edited

Choose a reason for hiding this comment

bilelmoussaoui commented Feb 18, 2022

SeaDve commented Feb 18, 2022

bilelmoussaoui commented Feb 26, 2022

SeaDve commented Feb 26, 2022

SeaDve commented Feb 26, 2022 • edited

SeaDve commented Feb 26, 2022

bilelmoussaoui commented Feb 26, 2022

Choose a reason for hiding this comment

SeaDve Feb 26, 2022 • edited

Choose a reason for hiding this comment

bilelmoussaoui commented Feb 27, 2022

SeaDve commented Feb 15, 2022 •

edited

SeaDve Feb 16, 2022 •

edited

SeaDve commented Feb 16, 2022 •

edited

SeaDve Feb 16, 2022 •

edited

SeaDve Feb 18, 2022 •

edited

SeaDve commented Feb 26, 2022 •

edited

SeaDve Feb 26, 2022 •

edited