Support additional application actions while registering an application

[azubieta]

This is a feature feature from AIL that is quite useful. Implementing it directly on the register method will save re-opening and re-editing the desktop file. Here is a draft of the method declaration.

            /**
             * @brief Register an AppImage in the system with additional desktop entry actions
             *
             * Extract the application main desktop entry, icons and mime type packages. Modifies their content to
             * properly match the AppImage file location and deploy them into the use XDG_DATA_HOME appending a
             * prefix to each file. Such prefix is composed as "<vendor id>_<appimage_path_md5>_<old_file_name>"
             *
             * The desktop entry actions must follow the specification for additional application actions at https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s11.html.
             * The map key should be the action identifier and the value the action fields in a plain string i.e.:
             *
             *  ```
             *  std::make_pair<std::string, std::string>("Remove",
             *     "[Desktop Action Remove]\n"
             *     "Name=\"Remove application\"\n"
             *     "Icon=remove\n"
             *     "Exec=remove-appimage-helper /path/to/the/AppImage\n");
             *```
             * @param appImage
             * @param additionalApplicationActions desktop entry actions to be added.
             */

            void registerAppImage(const core::AppImage& appImage, std::map<std::string, std::string> additionalApplicationActions) const;

[TheAssassin]

Been thinking a bit about this, and I think it'd be best to extract this into a separate class whose constructor takes the AppImage. Optional features like adding actions could be set via setters. There should be one command method that then performs the registration. That way, optional behavior can be used easily.

We can keep this method as-is (without the map) then as a high-level alias.

[azubieta]

We already have such class, it's the Integrator class. Such class was keep private for one reason: keeping the public interface simple. When the IntegrationManager was designed it was meant to be an abstraction that helps keeping the same configuration among all the "desktop integration features".

I think that we should honor such design decision and don't start adding lot's of classes to the libappimage public API. I'm thinking right now that more classes for thumbnails generation, un-integration and other might result from going that way you suggest. Which seems too much to me. So far re-editing the desktop files had worked well (there are no issues reported because of it) for adding experimental features, like adding application actions to the files. Therefore I would suggest keep using the same approach in such scenarios. If a given feature proves to be really required libappimage should already have all that's required to support it and it's implementation should not affect the public API. I will follow @probonopd rationale here about having too many switches, it's usually a bad idea.

Disclaimer: We would need a magic orb to know how a given piece of software will evolve in order take the perfect design decisions at this point.

[TheAssassin]

So your "solution" is to add 10 overloads for one method with different params?

[TheAssassin]

I think for 1.0 things are good enough as-is. But we should perhaps bear in mind such things for a future version 2.

[azubieta]

So your "solution" is to add 10 overloads for one method with different params?

Who said 10? My solution is to strictly add those features that are common and prone to errors or different behaviours when implemented by the clients. So regarding to the desktop entries only one more function overload will be required.

[TheAssassin]

One is clearly ambiguous to use. A method with more than one parameter taking a default is IMO bad design and really error prone. For yours to use I have to either fill something into the first map, or I have to pass an empty map and fill into the second, or pass two and really pay attention not to mix up. I can't see whether I'm doing it right though, without having a good IDE that shows me the parameter names and docs where the call is, because that's where I need that information to see if it's used right.

An object where I use setters with really obvious names isn't prone to errors, from the users' view at least.

Edit: That's something I don't like about Qt by the way. It surely keeps code somewhat compact, but it forces me to use some IDE to see if things are right. If I for instance look at code on GitHub I have big issues to validate calls. Hence I personally tend to use setters where possible with Qt.

It's an important design rule to only require really essential data in constructors, and allow for setting the rest with setters instead of constructor arguments. Functions are difficult here, as there's no way to use "setters" or anything. Some people pass "configuration objects" to functions to work around that limitation, but that then is clearly a workaround bad hack.