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

Remember installed verbs for a container #1824

Open
madoar opened this issue Feb 9, 2019 · 18 comments
Open

Remember installed verbs for a container #1824

madoar opened this issue Feb 9, 2019 · 18 comments

Comments

@madoar
Copy link
Collaborator

madoar commented Feb 9, 2019

Currently Phoenicis doesn't remember which verbs were installed in a container. This contains both the verbs installed via a script and the ones installed via the container tab.

To help the user know the current state of his container(s) it is a good idea to save the installed verbs for each container in a file in the container folder. The content of this file could then be used to:

  • skip the installation of already installed verbs when executing a script
  • display the already installed verbs in the container tab differently (e.g. disable the checkbox, add an "Already installed" text)
@plata
Copy link
Collaborator

plata commented Feb 9, 2019

Any preferred technical solution how to store this? Is there maybe a standard solution for this problem?

@madoar
Copy link
Collaborator Author

madoar commented Feb 9, 2019

I don't think that there is a standard solution. I think the easiest and best fitting way in our case is in a plain text file, maybe using the json format? @qparis do you have a suggestion?

@qparis
Copy link
Member

qparis commented Feb 9, 2019

We need a bean to read and write into a global configuration file. The implementation is nor important, it could be a local json file as long as we take care about race conditions

@plata
Copy link
Collaborator

plata commented Feb 10, 2019

Global? Wouldn't it be better per container?

@qparis
Copy link
Member

qparis commented Feb 11, 2019

Yep, same principle. We already have a config file per container

@plata
Copy link
Collaborator

plata commented Feb 11, 2019

Just to be sure ;)

@madoar
Copy link
Collaborator Author

madoar commented Aug 24, 2019

I would like to implement this soon. I think the issue can be divided in two parts:

  • changes in Phoenicis:
    • add two new methods to Engine:
      void registerVerb(String verbId);
      boolean isVerbRegistered(String verbId);
    • disable the installation checkbox in the container verbs panel when isVerbRegistered returns true for the given verb
  • changes in the Scripts:
    • implement the two new methods registerVerb and isVerbRegistered for Wine
    • add a registerVerb call to every Verb implementation in the scripts.

Open issues:
Currently the verbs have no knowledge about their script id which is required to call registerVerb. I think it would be useful to inject this information long-term in each script. For now I think it is ok to hardcode the script id into each script.

@ImperatorS79 do you want to help with the necessary script changes?

@ImperatorS79
Copy link
Contributor

disable the installation checkbox in the container verbs panel when isVerbRegistered returns true for the given verb

If a verb installation fails somehow, I would like to be possible to reinstall the verb. Maybe add a v next to the verb to say it is already installed ?

@madoar
Copy link
Collaborator Author

madoar commented Aug 24, 2019

I would suggest to call registerVerb as the last operation in the verb installation. This automatically means that the value is only set when the installation succeeds.

I think a v next to the verb name is more confusing to the user. A possibly approach could be the add a Reinstall button next to the script, but I think this should be part of a later PR.

@madoar
Copy link
Collaborator Author

madoar commented Aug 24, 2019

I just opened a PR with the Java/Phoenicis part (#2070).

@ImperatorS79 do you want to give the JS part a try?

@madoar
Copy link
Collaborator Author

madoar commented Aug 24, 2019

The new engine methods look like:

    /**
     * Registers as verb as being "installed"
     *
     * @param verbId The ID of the verb
     * @param container The container where the verb has been installed
     */
    void registerVerb(String verbId, String container);

    /**
     * Checks whether a given verb has been registered as being "installed"
     *
     * @param verbId The ID of the verb
     * @param container The container where the verb has been installed
     * @return True if the verb has been registered as being "installed", false otherwise
     */
    boolean isVerbRegistered(String verbId, String container);

@ImperatorS79
Copy link
Contributor

We could add the verb to phoenicis.cfg inside a container: verb: [verb1, verb2]. what the verb ID looks like ? I would like this to be still readable.

@madoar
Copy link
Collaborator Author

madoar commented Aug 24, 2019

The verb id is the script id, e.g. for adobeair it is engines.wine.verbs.adobeair

@plata
Copy link
Collaborator

plata commented Aug 25, 2019

Currently the verbs have no knowledge about their script id which is required to call registerVerb

I don't think it's a good idea to hard code the script IDs. This issue applies to all scripts. Maybe we could solve it with a ModuleInfo implemented in Java which returns information about the current or requested module? Somewhat similar to AppResource.

@madoar
Copy link
Collaborator Author

madoar commented Aug 25, 2019

We need a universal applicable solution that not only works for the scriptId but also for example for the categoryId and applicationId of the installer scripts.

I think your idea is quite could, let me extend it a bit. We could add an additional input to our module lambda function in:

String extendedString = String.format("(module) => { %s }", script);

We could change this to:

String extendedString = String.format("(module, ModuleInfo) => { %s }", script);

ModuleInfo could then be either a complete custom object, containing fields for e.g. scriptId, or a Map<String, String>.

I think for completeness we should also change our installer scripts to be modules.

@plata
Copy link
Collaborator

plata commented Aug 26, 2019

ModuleInfo could then be either a complete custom object, containing fields for e.g. scriptId, or a Map<String, String>

Could you give an example how this would look like in Javascript?

I think for completeness we should also change our installer scripts to be modules.

Yes.

@madoar
Copy link
Collaborator Author

madoar commented Aug 26, 2019

Inside the scripts you could access the module infos then like the following:

ModuleInfo.SCRIPT_ID // returns the script id
ModuleInfo.APPLICATION_ID // returns the application id (should only work for installer scripts)
...

@plata
Copy link
Collaborator

plata commented Aug 27, 2019

see #1242

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants