Bugfix package installation and revise server gui handling#345
Conversation
…ackage Host::installPackage(...) can call Host::uninstallPackage(...) which in turn can call Host::installPackage(...) back. This reentrancy could cause issues with a single instance of a QDialog used by the former which was accessed via a class member pointer. The previous code structure avoids this, but if the code failed to maintain the case where this is safe (when second argument has the value of 2) there would be multiple QDialogs created but only one could be tracked by a class member pointer. It is possible to avoid this issue by using a method local pointer which is enough in the new code which only uses the pointer in this method. The previous (void)Host::showUnpackingProgress(QString) was rendered ineffective with an immediate return statement. As such it was dead code and removing it also negated the reason for (void) mudlet::showUnzipProgress(const QString&) so that (int) TLuaInterpreter::showUnzipProgress(lua_State*) is also dead code. To avoid any breakage that has been converted into a stub that just reports via a nil + error message that it is such a stub and can be safely removed from scripts. Finally, the QDialog mentioned above was not being removed when Host::installPackage(...) failed with an error. This gave the effect of Mudlet hanging when package/module installation failed. Note that we are STILL woefully short of error messages in this process but that needs to be the subject of further commits. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…ditor The (bool)Host::serialize() method was short-circuited with an initial "return false;" and it was only called in: (void) dlgTriggerEditor::slot_save_edit() which did not rely on the return value. Further examination of the latter method also revealed that all of the dlgTriggerEditor::slot_saveXXXXAfterEdit() calls it used were just wrappers around the corresponding dlgTriggerEditor::saveXXX() methods and there was no other use of the "public slots:" wrappers so they can be safely eliminated. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Some file archiving production systems do not seem to be inserting the (zero length) entries with names ending in a '/' for sub-directories in archive files. This commit now makes two passes through any package or module and identifies all directories that will be needed, it then makes them before it extracts the files to populate those directories. This should fix https://bugs.launchpad.net/mudlet/+bug/1413069 as it also ensures that the proper cleaning process is done should package installation fail, it removes the "Unpacking" dialog on failure which was not happening before. NOTE: There is still an extreme lack of error reporting but as Host::installPackage(...) is used in several places by different parts of the application that is probably addressed separately. Also: * wrapped strings in Host::installPackage(...) in QStringLiteral(...) or tr(...) wrappers and made it more likely to be Utf8 clean. * Increased the size of the buffer used during extraction of possibly compressed archived package files from 100 to 4096 bytes which means less loops will be needed to process such archived files. * Ensured path / file name manipulations/test are not Case Sensitive so that they will not misbehave on MacOS platforms. * Tweaked displaying of the "Unpacking package/module..." dialog so that it shows more consistently (it often hid under other windows on my GNU/Linux setup!) Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
| .arg( _home ) | ||
| .arg( packageName ); | ||
| QDir _tmpDir( _home ); // home directory for the PROFILE | ||
| _tmpDir.mkpath( _dest ); |
There was a problem hiding this comment.
Previously setting _tmpDir to the profile directory ensures that any relative directory is resolved relative to the profile's home directory. Previously it would have been relative to the Application's execution directory which is OS dependent IIRC...
| { | ||
| pLabel->setText( tr( "Unpacking package:\n\"%1\"\nplease wait..." ).arg( packageName ) ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This puts the package/module name clearly in the centre of the dialogue rather than in the title bar where it is likely to be elided and not readable in the previous code!
| long long sum; | ||
| char buf[100]; | ||
| zip_uint64_t bytesRead = 0; | ||
| char buf[4096]; // Was 100 but that seems unduely stingy...! |
There was a problem hiding this comment.
Using a bigger buffer means many less iterations around for modules/package that are more than a few Kbytes in size! 😁
src/Host.cpp
Outdated
| moduleEntry << fileName; | ||
| moduleEntry << "0"; | ||
| mInstalledModules[packageName] = moduleEntry;//.append( packageName ); | ||
| moduleEntry << moduleEntry << QStringLiteral( "0" ); |
There was a problem hiding this comment.
💣 🪲 Arrgh! A copy/paste error - must fix it.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
If this is approved I will work to port/cherry-pick this to the release_30 branch. |
|
Unpacking dialog doesn't seem to work: |
|
Humm, that looks as though a |
…ectly I did not detect the dialog being put up but not being properly painted on my Linux box but Vadim did produce a screen capture video showing how the "Unpacking ..." message was being put up on top of the Mudlet application but the box, although the frame was displayed did not have it's internal contents filled in. It was not clear which OS platform he got this on and I did wonder whether it might have been a Mac which might have different GUI systems to my Linux one, but this commit ensures that a repaint() is used before a qApp->processEvents() call which should mean the widget is properly filled out... Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
@vadi2 I could not reproduce the issue that you showed in that screen-shot video, was it a Linux system or from your Hackintosh? I've put a |
|
That was on Ubuntu. Will check the fix! |
|
The issue is still present on Ubuntu. |
|
Are you testing this on a package big enough? For example my test case is 80mb, zipped. |
| pUnzipDialog->show(); // Must do this to ensure modality is applied | ||
| pUnzipDialog->update(); | ||
| qApp->processEvents(); // Try to ensure we are on top of any other dialogs | ||
| pUnzipDialog->repaint(); // Force a redraw |
There was a problem hiding this comment.
This forces a repaint of the Unpacking widget which is a mandatory redrawing of it - I cannot see why it is not getting repainted for you @vadi2, especially with the qApp->processEvents() call immediately afterwards which the system should be using to "refresh" anything else it thinks need repainting, etc. I'm baffled why it isn't being properly filled in for you...
The only further thing I can suggest is a:
if( pLabel )
{
pLabel->repaint();
}
before this line but I am grasping at straws here as I don't see why it isn't working for you - could you try it and see if that changes things?
There was a problem hiding this comment.
Same thing. Are you testing this with a big enough package for the dialog to show a while?
There was a problem hiding this comment.
I'm using a 5MB package and yes that is big enough for the text to flash up (completely filled out) for a short time. I was vaguely aware that there is the Qt::WA_OpaquePaintEvent that is used to tell Qt that a widget repaints it's entirety when redrawn so there is no need to erase it's background when redrawing it but that doesn't seem to be applicable to the QLabel involved in the :/ui/package_manager_unpack.ui UI file.
I've tried this on my Debian OldStable {NVidia graphics card using the latest Binary Drivers on that platform (3.40) IIRC} in both Debug and Release builds (in case the latter was optimising something that took out a redraw) with a g++ Version 4.7.2 using the qmake build system for Qt 5.7.0 and also a Debug build on Qt 5.0.2. I am using kdm and kwin so I believe I am using a compositing windowing system though I am using X11 and not Wayland. Is there any obvious difference to your Ubuntu system that might flag up something to cause the difference for you?
There was a problem hiding this comment.
Not really, though Ubuntu uses compiz. I've tried it on two different setups - one with proprietary nVidia and another with Intel, and the issue is present in both. Windows doesn't have the label issue though.
I'll test it on OSX and if it's not an issue there, we can park this minor UI issue.
There was a problem hiding this comment.
Works fine on OSX... feel free to merge
|
Will port to (release_30) branch ASAP. |
This fix will allow modules with sub-directories to be parsed and installed (which the prior code did not always do) as it scans archived packages/modules to establish any sub-directories needed and does not just rely on archive entries with a zero length file size ending in
/which is the defined method for indicating them - some (possible compressed) archive creation systems do not always seem to insert them.This should also make significant improvements to package/module installation as it finally cleans up the "Unpacking" dialogue that previously was left in view should the un-archiving or other operation fail - the previous failure to do this was giving the impression that the operation had hung rather than failed!
Whilst preparing this I found some chunks of ineffective or unused code that I excised as a preliminary commit. The code layout in the Host class is still wide of what I would like to see but that is a separate issue.
Note that although this now detects more error situation when working with packages it still does not report them. This will need separate work because the Host::instalPackage(...)
method is used in several different parts of the Mudlet application, they have potentially different ways to indicate problems and handling them here would make this PR significantly larger and thus harder to assess.
This should address the issues raised in Bug 1413069 which has been tagged as one of the 3.0.0 release blockers!