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

Refactorization and ui remake #89

Closed
wants to merge 92 commits into from
Closed

Conversation

@azubieta
Copy link
Collaborator

azubieta commented Sep 24, 2018

  • Move functionalities from shared.c and shared.h into more precisely named modules.
  • Move targets into separated directories inside "src/"
  • Remake Launcher UI interface.
  • Include additional information using libappimageinfo as source.
Alexis Lopez Zubieta added 20 commits Aug 30, 2018
@TheAssassin

This comment has been minimized.

Please finally change your IDE settings to not put a space between type name and */&. This will just cause me to make huge auto-reformat commits, which is quite unnecessary. For you, it's a simple 1-minute change in the settings.

@TheAssassin

This comment has been minimized.

Copy link
Owner

TheAssassin commented Oct 4, 2018

Still waiting for the integration directory to be displayed in the "Details" section of the main integration window.

@TheAssassin

This comment has been minimized.

Copy link
Owner

TheAssassin commented Oct 4, 2018

The new version removed support for content-aware filenames. It doesn't recognize overwriting of existing AppImages any more. Also, there is no need for a confirmation window when the integration worked. Just run the app. One less click. On errors, a message can be shown of course.

@azubieta

This comment has been minimized.

Copy link
Collaborator Author

azubieta commented Oct 5, 2018

Added the SHA 512 Checksum preview that you wanted and the label showing the Applications Directory path.

About the support for "support for content-aware filenames" I didn't do it intentionally, I was reviewing the code and all seems in place. If you recall where is it implemented in the old version please share the link. I'll look for it anyway.

About the confirmation message, it's good to let the user know that everything was ok, also it's used to provide additional information about how he/she can remove the AppImage later.

@azubieta

This comment has been minimized.

Copy link
Collaborator Author

azubieta commented Oct 5, 2018

Fixed: "support for content-aware filenames" please test.

Alexis Lopez Zubieta added 2 commits Oct 5, 2018
…orkflow.

The main argument behind this is to: Don't get into the users way.
@azubieta azubieta force-pushed the refactorization_and_ui_remake branch from 0f8526d to 895e141 Oct 8, 2018
TheAssassin added 3 commits Oct 9, 2018
Makes it look a bit more consistent.
@TheAssassin

This comment has been minimized.

Copy link
Owner

TheAssassin commented Oct 9, 2018

There's more workflow issues in the new codebase. That's the issue with huge refactorings... you can't really compare code.

If files already exist in the target directory, AppImageLauncher should ask whether to overwrite them. In that case, the old file could be rmed, and then the new file could be moved to the new location. Right now, there's the "brute force" fallback of copying it into the target directory. This breaks with the original UX: ask whether to overwrite the file, then try to move it, if that fails ask whether to try copying. That workflow was supposed to be extended with some "try with sudo" method in case of permission errors. This should be fixed.

@azubieta

This comment has been minimized.

Copy link
Collaborator Author

azubieta commented Oct 10, 2018

Just for the record. This also fixes #13

* Groups together the all the desktop integration functions.
*/
class AppImageDesktopIntegrationManager {
static QDir integratedAppImagesDir;

This comment has been minimized.

Copy link
@TheAssassin

TheAssassin Oct 10, 2018

Owner

Does this have to be static?

This comment has been minimized.

Copy link
@azubieta

azubieta Oct 11, 2018

Author Collaborator

It's used in static methods, so yes it has to.
If you wonder if this class could be improved? The answer is yes, but I already added too many changes to this pr. So I would let it for the next iteration.

@TheAssassin

This comment has been minimized.

Copy link
Owner

TheAssassin commented Oct 11, 2018

The new "content-aware filename" calculation is not complying with the old one. Please don't change such functions. You must revert to using the old one ASAP. https://github.com/TheAssassin/AppImageLauncher/blob/b04daee450bd79db47c60d7364a9a109d589222f/src/shared.cpp#L519-559

@azubieta

This comment has been minimized.

Copy link
Collaborator Author

azubieta commented Oct 12, 2018

Rolled back to the previous getAppImageDigestMd5 implementation.

@TheAssassin

This comment has been minimized.

Copy link
Owner

TheAssassin commented Oct 12, 2018

AppImageLauncher creates duplicate desktop files for the same AppImage. Also, the collision detection (i.e., the code that adds (1) etc.) doesn't provide multi locale support any more.

There's more and more problems, the code is not understandable any more (due to lack of comments, too many small functions that are just called once, etc.). I am tempted to reject this PR, merge the new UI(s) by hand and perform the refactoring afterwards. This is just not in a condition I could accept. The code was maintained externally too long, and it's not like there has just been a refactoring but also a lot of code has been replaced by incompatible "rewrites".

Don't get me wrong: I really like the new UI, the fact that libappimageinfo provides more information, etc. This really adds value to the software. But incompatibilities and lower reliability are not acceptable. And I don't get to a point where I'd feel familiar with the new codebase, there's no "obvious" structure any more, a lot of static code, there's no more comments that document the code (no, it is not "self-documenting", not at all). I don't believe these problems could be sorted out if we invest even more time into this PR.

@azubieta

This comment has been minimized.

Copy link
Collaborator Author

azubieta commented Oct 12, 2018

I'll take a look at the issues you mention.

Also, I would like to add that the previous state of the code not fully documented and much fuzzier. Therefore the workflow cannot be guessed properly. Also, there are no unit tests to validate this workflow. You just feel more comfortable on it because it was written by you.

The term obvious depends on what are you used to. The static code is harmless and there was more before (like the whole main.cpp file is pure static code). I tried to make the code to express its intention in that way it would be 'self-documented'. In the cases that this is not achieved we can just use a new name which is very easy and harmless to do using a proper IDE and if this is not enought we cand add coments (but comments outdate easily and cannot be tested). There are a lot of small functions because each functions tries to do only one thing at the time, so it's easier to understand.

Let's make an architectural review to give you a better piture of the resulting project structure:
As we know this project provides two tools that fulfill different workflows for integrating AppImages into a desktop environment: AppImageLancher and launcherd.

AppImageLauncher intercepts the executions of the AppImage file by installing a Desktop mime-type handler and a binary format (binfmt.d) handler to perform AppImage integration. Then it shows a dialog with information about the AppImage being opened and provides the options to "Integrated the AppImage into the desktop and run it" or just "run it".

launcherd monitors certain directories to perform AppImage integration. Newly add files are integrated and remove files are desintegrated into the desktop in the background.

Both tools use the same desktop 'integration' and 'desintegration' code, therefore, it was placed in a class named AppImageDesktopIntegrationManager that is used by both. It reads its configuration using the AppImageLauncherConfig class which handles the configuration persistence. Next to this class were defined as a group of custom exceptions that will be thrown when something goes wrong or out of the regular workflow.

As each tool provides a different UX the code related to it is split. In src/daemon folder is placed the appimagelauncherd related code and in src/launcher is placed the AppImageLauncher code.

The appimagelauncherd code didn't suffer many modifications so I'll not go deep on it.

To fulfill AppImageLauncher UX were created two classes: Launcher and UI.
The Launcher class provide the methods to implement the UX in a user interface abstract way.
The UI class provides a GUI to the Launcher class. Its main entry point is UI::showIntegrationPage(). It's important to mention that it doesn't implement any business logic that's all in the Launcher class.
In the 'main' file were kept following the workflows:

  • choosing where to execute a GUI application or a CLI application.
  • TrashBin cleanup
  • early validation of the application arguments
  • early validation of the AppImage and notification of possible errors
  • choosing if the AppImage will be "handled" or just executed.

In resume: AppImageLauncher now looks like this:
main -> UI -> Launcher -> AppImageDesktopIntegrationManager

In general, don't panic. If more problems show up, as they will. I'll be here to deal with them.

@TheAssassin

This comment has been minimized.

Copy link
Owner

TheAssassin commented Oct 15, 2018

Closing PR as unmergeable. Merging portion wise by cherry-picking similar commits into reasonable "bundles" in branches, and sending multiple PRs. Fixing left-over issues in the progress.

Note for self: do not delete this branch yet

@probonopd

This comment has been minimized.

Copy link
Contributor

probonopd commented Oct 24, 2018

I think this is a great UX improvement:

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.