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

Desktop: Remove demo plugins folder and update pinned version of backup plugin #6801

Merged
merged 2 commits into from
Sep 5, 2022

Conversation

mak2002
Copy link
Contributor

@mak2002 mak2002 commented Sep 2, 2022

There were some plugins' .jpl files that were included in this PR for demo purposes. But we don't need them anymore, as now we have the script that will bundle the necessary plugins. Apart from that, the demo version (included currently) of 'Simple backup plugin' does not create sub-folder automatically causing pop-up on initial startup.

If we don't remove this folder, the bundling script will throw an error as manifests of these plugins don't exist.

@laurent22
Copy link
Owner

Apart from that, the demo version (included currently) of 'Simple backup plugin' does not create sub-folder automatically causing pop-up on initial startup.

Ok, but I mean this is quite important, it's an app that's going to be used by many people. So what do you want to do about this? We don't want a pop-up on startup. And what is this demo plugin - that's not even the real Backup plugin?

If we don't remove this folder, the bundling script will throw an error as manifests of these plugins don't exist.

I don't know what "these plugins" are, please clarify. And do you mean that the bundling script doesn't even work at the moment? Then we can't even build a release? Please understand that what we merge is what ends up in production, we can't have demo files and errors being thrown in there.

@Daeraxa
Copy link
Collaborator

Daeraxa commented Sep 2, 2022

My understanding of the issue is that #6585 featured two erroneous .jpls that were being used for testing and this PR is simply to strip out those erroneous files rather than having to go through rolling back the previous PR and resubmitting.

@mak2002
Copy link
Contributor Author

mak2002 commented Sep 2, 2022

Daeraxa, you are right.

Ok, but I mean this is quite important, it's an app that's going to be used by many people. So what do you want to do about this? We don't want a pop-up on startup. And what is this demo plugin - that's not even the real Backup plugin?

It's real 'Simple Backup plugin' just not the version we wanted. Version 1.1.1 won't cause pop-up as it will auto create sub-folder in given backup path which we will be setting from the app itself. I have updated the plugin's version now.

I don't know what "these plugins" are, please clarify. And do you mean that the bundling script doesn't even work at the moment? Then we can't even build a release? Please understand that what we merge is what ends up in production, we can't have demo files and errors being thrown in there.

I was referring to the demo plugins as "these plugins" here. At the moment, script will throw an error becuase manifest files of demo plugins does not exist which is required by bundling script to get local plugin versions.

But after removing these demo plugins, bundling script will run fine.

Sorry about all this, I will be definitely more carefull from now onwards.

@CalebJohn
Copy link
Collaborator

Just to add a touch more clarity here. There are 2 changes in this PR

  1. Removing demo .jpl files
    In order to test (and allow reviewers to test) the default plugin feature makb included some demo .jpl files in the branch. He forgot (or maybe didn't get a chance) to remove these files before merging, so they accidentally ended up in master.
    This PR simply deletes them.
  2. Update backup plugin manifest version
    Older versions of the backup plugin pop a message to the user when a path hasn't been set. This is not acceptable for a default plugin. Makb has worked with Jack to ensure that the backup plugin won't do that in future versions (1.10 onwards).
    This PR updates the pinned backup plugin version to the latest, which ensures the popup is no longer present.

@mak2002 mak2002 changed the title Desktop: Remove demo plugins folder Desktop: Remove demo plugins folder and update pinned version of backup plugin Sep 2, 2022
@laurent22
Copy link
Owner

Thanks everyone for clarifying, much appreciated. That makes sense now, so let's merge

@laurent22 laurent22 merged commit 80906cb into laurent22:dev Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants