BugFix: allow packages with sub-directories & get exporter to report result by SlySven · Pull Request #2554 · Mudlet/Mudlet

SlySven · 2019-05-16T18:39:44Z

The previous code would silently fail when using zip_file_add(...) if the item to be added was actually a directory. This commit fixes that by examining each "file" that is to be added to the archive file and deduces all sub-directories that need to be made and arranges for those to be inserted (with zip_dir_add(...)) first before populating the archive with the actual files.

It also refactors the code so that a pass/fail message is displayed at the end of the package exportation operation - and so that the dialogue is not closed until the user has clicked on a suitable button ("Cancel" before the package is exported, "Close" if the process failed and "Okay" if it succeeded). The revised success message also retains a suggestion (with a clickable link) to upload the ".mpackage" to the appropriate Mudlet forum topic.

Whilst debugging this I also found:

(bool) XMLexport::saveXml(const QString&) was not returning a true on success - which as the code now checks and reports on failures needed addressing.
there were some unused elements on the form/dialogue that seemed to be left over from a previous implementation that could be removed.
there was an unused dlgPackageExporter constructor that could be removed
some class members were not obviously named as if they were - indeed one shared the same name as an element on the form/dialogue.
there was an element on the form that could show the target name and path of the package but it was being hidden - it is now shown whilst the user is selecting the Mudlet items and extra files to include - but is hidden on successful package creation as the message for the latter includes the same piece of information.
the dialogue that prompts for the package name had a pointless duplication of title and prompt texts - the latter has now been revised.
the dialogue that prompts for the package save location was not being supplied with a correct argument - instead of a starting path it was being given the text "Where do you want to save the package?" which is never (with the '?') going to be a valid path!!!
that same dialogue was also not showing the files in the directory - and as the code is quite capable of overwriting an existing archive it makes sense to show files as well as directories when selection the latter.

The Qt Documentation suggests that on the Windows platform it is not possible to show files in a directory selection dialog using the native OS dialogue - I also found this was the case with the KDE Plasma Desktop Environment so I have set the options NOT to use a native dialogue for all platforms, not just the Windows one.

The packages created now have the intended .mpackage extension though they are .zip format archives underneath.

This should close #2547 and also close #1844 .

Signed-off-by: Stephen Lyons slysven@virginmedia.com

…result The previous code would silently fail when using `zip_file_add(...)` if the item to be added was actually a directory. This commit fixes that by examining each "file" that is to be added to the archive file and deduces all sub-directories that need to be made and arranges for those to be inserted (with `zip_dir_add(...)`) first before populating the archive with the actual files. It also refactors the code so that a pass/fail message is displayed at the end of the package exportation operation - and so that the dialog is not closed until the user has clicked on a suitable button ("Cancel" before the package is exported, "Close" if the process failed and "Okay" if it succeeded). The revised success message also retains a suggestion (with a clickable link) to upload the ".mpackage" to the appropriate Mudlet forum topic. Whilst debugging this I also found: * `(bool) XMLexport::saveXml(const QString&)` was not returning a `true` on success - which as the code now checks and reports on failures needed addressing. * there were some unused elements on the form/dialog that seemed to be left over from a previous implementation that could be removed. * there was an unused dlgPackageExporter constructor that could be removed * some class members were not obviously named as if they were - indeed one shared the same name as an element on the form/dialog. * there was an element on the form that could show the target name and path of the package but it was being hidden - it is now shown whilst the user is selecting the Mudlet items and extra files to include - but is hidden on successful package creation as the message for the latter includes the same piece of information. * the dialog that prompts for the package name had a pointless duplication of title and prompt texts - the latter has now been revised. * the dialog that prompts for the package save location was not being supplied with a correct argument - instead of a starting path it was being given the text "Where do you want to save the package?" which is never (with the '?') going to be a valid path!!! * that same dialog was also not showing the files in the directory - and as the code is quite capable of overwriting an existing archive it makes sense to show files as well as directories when selection the latter. The Qt Documentation suggests that on the Windows platform it is not possible to show files in a directory selection dialog using the native OS dialogue - I also found this was the case with the KDE Plasma Desktop Environment so I have set the options NOT to use a native dialog for all platforms, not just the Windows one. The packages created now have the intended `.mpackage` extension though they are `.zip` format archives underneath. This should close Mudlet#2547 and also close Mudlet#1844 . Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

add-deployment-links · 2019-05-16T18:39:55Z

Hey there! Thanks for helping Mudlet improve. 🌟

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

Kebap

Holy guacamole! This is not merely a bugfix but a massive commit once again and could be easily split into 10 seperate PRs for easier digestion. Was this coded over a longer period or in one short session now? Will need time for testing... Now any discussion on a singular concept changed will block the other 9 from merging, even though they may be totally uncritical, therefore reducing the total time available for testing.

Kebap · 2019-05-16T22:13:22Z

src/dlgPackageExporter.cpp

+{
+    struct zip_source* s = zip_source_file(archive, fileSystemFileName.toUtf8().constData(), 0, -1);
+    if (s == nullptr) {
+        displayResultMessage(tr("Failed to source file \"%1\" to place into module (archive) file, error is: \"%3\".")


Hard to grasp. Why module / archive and no package mentioned?

Revised to say package (and give a bit more information to the translators).

Kebap · 2019-05-16T22:13:36Z

src/dlgPackageExporter.cpp

+    }
+
+    if (zip_file_add(archive, archiveFileName.toUtf8().constData(), s, ZIP_FL_ENC_UTF_8|ZIP_FL_OVERWRITE) == -1) {
+        displayResultMessage(tr("Failed to add file \"%1\" to module (archive) file \"%2\", error message was: \"%3\".")


src/dlgPackageExporter.cpp

Kebap · 2019-05-16T22:29:22Z

src/dlgPackageExporter.cpp

+                    // If successful will get to HERE...
+
+                } else {
+                    displayResultMessage(tr("Required \"%1\" file that contains the mudlet items chosen for the "


Required "name" file vs. Required file "name". Also run-on sentence could be split easily in 3-4 parts to ease translatability. Again not sure why we keep mentioning "module" and "module (archive)" here, as there should be a single consistent word used and for me that is package.

I must confess that I have a conscious bias in calling .xml files containing a single type (trigger, timer, key-binding, etc.) of Mudlet item a package whereas the output from this code which can contain more than one type of Mudlet items and that can also contain other files a module.

The mentioning of archive is because libzip is being used to handle what is actually an .zip archive type file and in some circumstances it makes sense (to me) to say that to explain what is (or is not) happening - most people understand what a zip file is (a file that contains lots of other files that have been squashed up inside it).

Actually, .zip was the file extension being used for what was being produced until this PR. This one finally starts using the .mpackage extension that we have been coding for since Chris's commit f98be22 in 2013! 😮

I will take a look at the wordings though to see if I can make my sentences shorter...

That's not correct though - the only difference between a package and a module is how it is installed in Mudlet.

So we do not distinguish between:

the item that is a single XML file contain only one type of Mudler item and made from the "Export" button in the editor

the item that is an archive file containing at least an XML file of maybe more then one type of Mudlet item, a config.lua file that currently + records the module name (but which is retained internally in the (QString) Tree::mPackageName and NOT the unused (QString) Tree::mModuleName item members) and any other files that the creator wants included and is made from the (misnamed I feel) "Package exporter (experimental)"

❓

+ - I think we can extend that file to also include other things like the creator/licence (?)/creation datetime stamp/version number/save number and - to solve #659 the helpURL link...

That's right, we don't (and no need to!)

But, but, but they are just not the same as far as I can see. 😮

For instance you can (if you were brave enough open the XML form in a text editor and hack it by hand) if you open an archive file directly in something like notepad++.exe you will not get anything you can use - you have to open the file with something like 7Zip.exe and then work on the files within the archive {not sure if Windoze explorer.exe is clever enough to spot that an .mpackage file is a zip archive!}

The name that is displayed in the editor for the module (or package as you insist) {the brown folder} need not be the same as the filename it is loaded from for the case using an archive file but it is forced to be that (without the .xml {or .mtrigger}) for the simple XML case.

All this could be discussed in wiki to some extent, but can't be extensively in a tool-tip or error message. It is only confusing most users to see "module (archive) file" and no "package" mentioned in a "package exporter" functionality...

Reworded (to include package instead of module), broken up into more sentences, and lack of space added as a possible reason for the problem alongside that of permissions.

Ah:

"... that contains the mudlet items ..."

Should I have it as Mudlet items?

Kebap · 2019-05-16T22:30:38Z

src/dlgPackageExporter.cpp

+                // details):
+                ze = zip_close(archive);
+                if (ze) {
+                    displayResultMessage(tr("Failed to populate and then close module (archive) file, error is: \"%1\".")


populate and then close is very tech-focused and correct, but more understandable would be write. Also, package?

I'd want to review the other texts to check whether the word "write" is used in them - I would rather like to keep the error messages as distinct as possible so that if a user moans about something going wrong for them we can work out precisely where something went wrong.

I've been through the messages and converted module to (.zip format archive) package file - which seems to more precisely define the item - does that make better sense?

src/dlgPackageExporter.cpp

vadi2

Pending @Kebap's feedback, looks good to me!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

I had lost the third '/' from the "file:///localPathName" argument to the openUrl method so it was not working when the "add files" button was clicked in the package exporter. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Kebap

Functionality seems fine after some more testing done in Discord.
Additional ideas will be / have been filed as new issues to prevent scope creep here.
However, the wording of messages did not improve too much, I fear.
Some comments for translation seem more like comments for developers instead.
To speed up this back-and-forth exchange, I propose a PR to your branch with my adjustments.

Kebap · 2019-06-21T05:56:43Z

src/dlgPackageExporter.cpp

+    if (s == nullptr) {
+        displayResultMessage(tr("Failed to source file \"%1\" to place into (.zip format archive) package file, error is: \"%3\".",
+                                // Intentional comment to separate arguments
+                                "This error message will appear when a file is to be placed into the .zip type file (though we give it an .mpackage extension) but the libzip library cannot open it.")


You changed message from
Failed to source file \"%1\" to place into module (archive) file
to
Failed to source file \"%1\" to place into (.zip format archive) package file
so it got even longer. The trick is to make it more concise instead, so translators have less words to work with, and users have less words to comprehend.
Also you added a translation comment of
This error message will appear when a file is to be placed into the .zip type file (though we give it an .mpackage extension) but the libzip library cannot open it.
which does give additional detail about the filetype of the file to be produced, and the library being used, etc. which don't help translators at all, I guess.
A good addition there is to name it "open" a file instead of "source".
I would remove all mentions to filetype and library (which keep appearing throughout the following messages as well) and instead just display a message like:
"Failed to open file \"%1\" to place into package file. Error is: \"%3\".

I will send a PR to your branch to make this easier.

The niggle with that it that it is not clear that it is actually opening the file at that point - it seems that the libzip code is making some sort of record of the file to be added (sourced?) - but it does not actually get processed until the zip file is zip_close() ed at the end of the method - which (as I have pointed out elsewhere) can take a noticeable time.

You see, for me, as a non-native, I wouldn't even know what niggle means, let alone how to translate it, or to come back closer to the matter at hand, a "source file" is a very well-defined concept, but "Failed to source file" leaves me puzzled.

What I am trying to say is: There is a difference between the level of detail necessary for a mere user using a software, and the developer developing it. The delicacies of libzip library would fall into the latter category for me. Something is wrong with the %1 file which was intended to be opened at some point in this operation. That seems about enough info.

Also I changed %3 to %2 in this message. Again, all details in said PR linked to below.

📖 Actually Qt will handle the absence of a %2 just fine - it automagically looks to replace the lowest numbered unhandled parameter one by one and will only throw a fit if there is not enough arguments by the time it gets to the last one (can't recall how it behaves if there are too many arguments - it might get unhappy as well then or it might just ignore the excess...) - OTOH I will tweak that before I merge this as it otherwise is...

Kebap · 2019-06-21T05:58:33Z

src/dlgPackageExporter.cpp

+    if (!writer.exportPackage(mXmlPathFileName)) {
+        displayResultMessage(tr("Failed to export - could not write Mudlet items to %1.").arg(mXmlPathFileName),
+                             false);
+        // Although we have failed we must not just abort here we need to reset


This seems not adressed, yet.

src/dlgPackageExporter.cpp

Kebap · 2019-06-25T14:14:14Z

I am not sure about the best way forward.
The functionality seems fine as stated above.
We could ship as-is and then I create a new PR.
Just like I have now created a PR in SlySven's branch.
This takes care of updating all the ui texts mentioned above.

SlySven · 2019-06-25T18:40:13Z

This seems not adressed, yet.

Not sure what the issue there was now...

Kebap · 2019-06-25T18:43:40Z

Please review here: SlySven#5

The `QString::arg(...)` method will not actually have a problem with this but the missing number might be confusing. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2019-06-25T21:09:51Z

@Kebap Can you propose that PR onto the main development branch now, please...

SlySven requested a review from a team as a code owner May 16, 2019 18:39

SlySven requested a review from a team May 16, 2019 18:39

SlySven assigned vadi2 May 16, 2019

SlySven added 1 - Ready for review medium labels May 16, 2019

SlySven added this to the 3.21.0 milestone May 16, 2019

Kebap reviewed May 16, 2019

View reviewed changes

vadi2 approved these changes May 19, 2019

View reviewed changes

This was referenced May 23, 2019

Adjust link to upload forum #2561

Closed

Package exporter should link to created file #2563

Closed

vadi2 assigned Kebap and SlySven and unassigned vadi2 May 24, 2019

Kebap removed their assignment May 29, 2019

Kebap added 0 - Work in progress and removed 1 - Ready for review labels May 29, 2019

vadi2 removed this from the 3.21.0 milestone Jun 16, 2019

Revise: following peer review

00009c2

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 assigned Kebap and unassigned SlySven Jun 18, 2019

vadi2 added 1 - Ready for review and removed 0 - Work in progress labels Jun 18, 2019

SlySven mentioned this pull request Jun 19, 2019

Package exporter should remember where the last package was exported to. #2627

Closed

BugFix: get addFiles action working

76e442b

I had lost the third '/' from the "file:///localPathName" argument to the openUrl method so it was not working when the "add files" button was clicked in the package exporter. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Kebap requested changes Jun 21, 2019

View reviewed changes

Kebap mentioned this pull request Jun 21, 2019

Adjust messages for users/translators SlySven/Mudlet#5

Closed

vadi2 approved these changes Jun 25, 2019

View reviewed changes

Kebap approved these changes Jun 25, 2019

View reviewed changes

BugFix: correct parameter numbering error

d6b9cc0

The `QString::arg(...)` method will not actually have a problem with this but the missing number might be confusing. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven merged commit 30450a5 into Mudlet:development Jun 25, 2019

SlySven deleted the BugFix_allowPackagesWithSubDirectories branch June 25, 2019 21:09

Kebap mentioned this pull request Jun 25, 2019

Rephrase package exporter #2659

Merged

SlySven restored the BugFix_allowPackagesWithSubDirectories branch June 22, 2020 17:57

SlySven deleted the BugFix_allowPackagesWithSubDirectories branch June 22, 2020 18:50

Uh oh!

Conversation

SlySven commented May 16, 2019

Uh oh!

add-deployment-links bot commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kebap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Kebap left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kebap commented Jun 25, 2019

Uh oh!

SlySven commented Jun 25, 2019

Uh oh!

Kebap commented Jun 25, 2019

Uh oh!

SlySven commented Jun 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

add-deployment-links bot commented May 16, 2019 •

edited

Loading

Kebap left a comment •

edited

Loading