General module/package system overhaul#1355
General module/package system overhaul#1355SlySven wants to merge 21 commits intoMudlet:developmentfrom
Conversation
Also includes error/success reporting (as the package exporter dialogue
no longer disappears as soon as "Export" is clicked).
Code refactored to support both 0.1x and 1.x versions of libzip - former
is currently only option on the current Linux Travis C.I. {uUbuntu Trusty
Tahr LTS} platform but later forms have error handling code that is
resistant to the string buffer overflow vulnerabilities in the older code.
Includes option to use a properly "temporary" staging area that will
dispose of itself once the module has been zipped into an archive file but,
to ensure compatibility with previous implementations it is not the default
option each time a module is made.
There is scope to enhance the use of the config.lua file to allow
additional details besides the module name to be stored in that file. I
have left TODO: markers where such detail could be processed. In the past
I have experimented with code that included the following (optional data):
* Version (Symantic form?)
* Author name
* Email
* URL
* Creation Date (auto filled in)
* Last Saved Date (for synced modules)
however that is beyond the scope of this PR and requires additional
handling in the Host class at least.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
The absence of error messages from code using the libzip library to handle archive files has been an issue for some time - this commit inserts a whole lot of them and makes use of libzip 1.x enhancements (particularly in error message string handling) when it can. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Also needed to revise some infrastructure to have optional capability to pass error messages back up caller chain (to Lua caller) rather than pushing error messages to the main profile's TConsole. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
Blast, |
We can search for a PPA that has a newer libzip and up the requirement, no worries. |
|
And the AppVeyor CI is using a 0.11 tarball from Canonical's Ubuntu site and not from the creator's own |
It was set to use ancient 0.11.2 and from the Ubuntu packages repository, now it will fetch from the creators' official site. Also added new CI related files to QMake project file for convenience. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
… testing In order to check that some code for downgraded versions of libzip was tested for compilation on my development platform I deliberately negated some #if tests in order to force sections to be included in the compiled source code when they would not normally be. I had forgotten to undo them causing some issues when the CI systems tried to build my code... Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
That is weird - |
|
Strange, but perhaps it'll get fixed when #1356 is in. |
I suspect that changing the libzip version being used from 0.10-ish to the latest 1.30 needs to change the SO version number from 2 to 3 as the libzip/CMakeLists.txt seemed to change what I think causes the "dll" to be "libzip-2.dll" immediately before 0.10 was released. See: nih-at/libzip@87628b4 Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
💭 When(if ?) this gets approved I'll squash and merge it in so that the fixes for the build infrastructure can be ported into # 1356 before-hand and then they will be silently fast-forwarded over and disappear from this PR... |
… CI(2) I suspect that changing the libzip version being used from 0.10-ish to the latest 1.30 needs to change the SO version - try again as 3 didn't work - this time with 4! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
I'll work on getting it reviewed, but you've gotta keep the PR size down. Break them down into digestible chunks that do one thing at a time. |
|
💡 You may find it more digestible to consider the first three commits successively, perhaps? |
|
One way to make PRs more digestible is to do only one thing at once. The PR title already suggests you did two things. |
|
In my defence I would offer the opinion that It was not so easy to consider the matters addressed separately - there were issues in the error handling (there wasn't much of it) and work was also needed in the package creator (part of it was how to show those errors)...! - But yeah I do get that it is not as nice and simple as some other PRs out there. |
|
👍 We love your stuff, you know that. Just trying to make it more approachable to work with 😉 |
|
Ah, glad that at least someone else besides me doesn't think it is a steaming pile of 💩 ! |
…or on Appveyor CI(3) I suspect that changing the libzip version being used from 0.10-ish to the latest 1.30 needs to change the SO version - try again as 4 didn't work - this time without a number! FTR: Also as I know the current code will not build on Travis I am using a "tag" in the top line to prevent it but hopefully NOT AppVeyor to not attempt a build - the corresponding command for the latter seems to be to replace "ci" with "appveyor" in that "tag". Had to redo this commit (and overwrite history!) as the previous version had a tag that stopped BOTH platforms from building the code and to fix it needed a change to the commit message ONLY! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
fe9d3e5 to
7c2f80e
Compare
|
I don't think any of your contributions is useless. Quite far from it. Each of your PRs brings us a huge step forward. I didn't want to imply anything less. If you read anything different than what Vadi said above I have to apologize profusely. |
|
😊 Ooops - I didn't mean to give anyone the impression that I felt got at. No apologies are sought/needed. |
…or on Appveyor CI(4) I suspect that changing the libzip version being used from 0.10-ish to the latest 1.30 needs to change the SO version - one last try again with a number plus letter combination that I thought I saw: "3a" Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
No... that's not a good solution. Sorry, but spamming the user with error messages just frustrates people! Yes, this is another Frankenstein PR, unfortunate, but we can't make Mudlet annoying to use, that would be a regression. A user-friendly way would be to show the notification where it is relevant - in the module manager, not in the main screen always on startup. |
Actually that is by design: I deliberately ensure that another module is NOT automatically selected (and that the
Ah, that is a side effect of the module/package code being usable from:
If the code is invoked from lua it'll get a nil+error message otherwise it will appear on the permanent record that is the main profile console because there is not anything to show the error on until the module manager is accessed for the profile affected! I suppose there might be a way to store the status/error messages for each module when loading is attempted and if there is problem we could change the icon on main toolbar - no that wouldn't work because the button is used by the active profile and it can be hidden. A status icon could go on the bottom buttons - no that wouldn't work because those too can be hidden. I guess it might be possible to put such an icon on the tab bar if there is more than one profile active. 🤔 There is not a universal simple solution to this... |
That is actually really annoying - when you have to uninstall 10 modules, you should be able to just click 'UnInstall' 10 times. Besides, if you uninstall a module accidentally, there should be no data loss - you just install it again and you are good. The proper way to handle this is to offer an easy 'undo', that is the proper way: instead of trying to prevent people from making mistakes by making the program hard to do, which doesn't work because people will still make mistakes, make it easy to fix mistakes. Not asking for an undo here because that's a lot of work, but it should be possible to uninstall a bunch of modules easily without it being a pain. |
And that is just fine. You see something is wrong with your modules, you go look in the module manager. Don't need to show anything in the toolbar or anywhere else, it's not relevant there, and not all module load errors are errors you'd be interested in fixing. |
|
Given that this is a "module/package system overhaul" - I'd like to propose that newly-installed modules are synced by default. I've started to seriously work with modules in the last few days and the amount of times I have lost my work because sync is not on by default is frustrating. Why would we even have "data loss on" as a default policy? |
You would have to have extra code to detect if the module is installed in another active profile and then give the user control over in which profile the module is marked as being synced - otherwise there will be data loss when they both try to save their revised version - the unsynced (and thus unsaved by default at profile shutdown) setting avoids this complication by leaving it to the user to manually activate the option - at least now there is warning text to say why it is not a good idea to set the sync option on multiple instances...
Well it is not quite such a negative thing in that if the module comes from someone else it prevents local damage - any changes to the module gets reset/reverted the next time it is loaded. Not so hot for module developers as it is easier to work on an un-package/module instance where the top level containers are normally activated and the module version is only loaded in for testing (and then the first set is disabled) - but for end users it helps to keep them using the module as distributed... |
"You see something is wrong with your modules": how will you know that there is something wrong to go looking? " not all module load errors are errors you'd be interested in fixing": what criterion should be applied and for whom? Fair enough, just dumping everything to the main console is not new(?)-user friendly but neither is hiding it away in somewhere they may not think to check... 🤔 Also there could be a chicken-and-egg situation: putting an icon against modules with issues in the module manager only works for modules that have been loaded - if there are significant errors that mean they cannot be loaded then there will not necessarily be enough information from the loading process in the current data structures (which may not be instantiated for the module file concerned) that could be displayed... |
|
With regard to the serial deletion of modules and/or packages I think it would be a reasonable compromise then to allow the next item in the list to be left as the selected one - but to ensure the default button (the one that gets activated if "Enter" key is pressed) is the "Close" button rather than either "Install" or "Uninstall" - I'll try and code that in ASAP... |
|
💡 Okay, I have an idea to add a second |
If you don't know, it's not a problem!
I'd say such "epthemereal" modules should always show up, so you can remove them if you don't want to deal with fixing the problem... otherwise you are forced to fix the problem or see the error messages all the time.
Could work as an easy-to-code solution. Not sure how aesthetic that would be though. |
|
Uuurgh, been working my way through checking that potential error messages from |
|
Could you add a splitter above the giant info message, so you could make it smaller / hide it? Seeing it for the 100th time and having it take up half of the screen so I have to scroll a lot kind of gets old, now that I'm using the module manager more! :D |
|
Closing as there's been no progress for a year... let's pull the functionality into non-mega PRs? The branch with the code will still be there! |
Brief overview of PR changes/additions
This PR should enable us to take the experimental marking off from the package exporter menu entry. It addresses issues I have found with using the package/module exported when there are additional files in sub-directories to include in the module (in the past this would fail with no error notifications).
It updates the
dlgPackageExporterclass to permit a proper temporary directory to be used for the staging area where all files for a module/package are assembled (but mindful of the past behaviour it does NOT default to working in that manner) and to respect a past Schrödinbug which due to a past coding error means the current code does not default to overwriting an existing module/package archive file with the same name (the prior code would reopen the previous archive but without theZIP_TRUNCATEoption to blow away the existing content it was coded in a way that would fail to replace existing files in the existing archive as the obsoletezip_add(...)function does not do that andzip_replace(...)should have been used) - this too can now be specified when the package/module name is collected. To allow the user to check for these situation the location to save the module/package to no longer uses the native file dialog and instead forces the use of one that shows the files as well as directories (which the Windows native one does not allow).The main dialogue/form for selecting the items to include in the module/package has been reworked so the UI is more functional - it no longer disappears when the "Export" button is click - instead a colour coded results message is shown under the now displayed (was hidden) full path to the module file that will/is being created at the same time the export button is locked down and that and the "Cancel" button are disabled and the previously disabled "Close" button is enabled.
The text in the above has been revised to reflect that other fixes have been made which means that archive file based modules are now updated when they are set to be "Synced" in the module manager - should Mudlet items be added to or removed from the (brown) folders of modules in the editor then Saving the profile to the file-system (at the end of the session) or via the Editor control WILL REWRITE the
<moduleName>.xmlfile in the<moduleName>.mpackagearchive file - though only the latter save action will cause other profiles to immediately reload the revised content. Note: this does not backup themoduleName.xmlfile anywhere - which is different from the manner in which single-filepackageName.xmlpackages/modules are handled. Those get saved as a dateTimeStamped suffixed version in the<mudletHomeDir>/modulesBackup/directory (shared between profiles). This does mean that setting the "Synced" option for a module in more than one active profile is a bad thing because only the changes in the last profile saved at the end of the session (or the last one to be closed) will make it into the (shared) module archive file. This is implicit in the current design and a mention of this has been made in the module managed displayed info-text.The lua commands
installModuleinstallPackageuninstallModuleuninstallPackagereloadModulehave been updated to return a true or nil+error message result and the error message should include the error details from the C++ core or the
libziplibrary as appropriate. Some tweaking of the C++ core was necessary to add in the missing error message generation. This also showed up that, like the Travis CI Linux Ubuntu Trusty Tahr LTS platform we were using obsoleted code from the 0.1x versions oflibzipwhich has a few issues. I have included conditional compilation code chunks to use the current 1.xlibzipwhen it is available which has better support for error messages (no need for finite lengthchar[]arrays for them with the dangerous buffer overflow vulnerabilities that introduces) and confirmed UTF-8 internal file name handling.Motivation for adding to Mudlet
In addressing this (which I started in #1334 but which the code here supplants) I finally attempt to fix something that has been bugging me for a while in that there was almost a complete absence of error reporting in much of the C++ code that handles archive files (the
.zipand Mudlet specific alias.mpackagefile types).Other info (issues closed, discussion etc)
To follow...
Should finally close #566! 😀