Skip to content

Fix package exporter to work with async save#1832

Merged
vadi2 merged 3 commits intoMudlet:developmentfrom
vadi2:fix-package-exporter
Jul 25, 2018
Merged

Fix package exporter to work with async save#1832
vadi2 merged 3 commits intoMudlet:developmentfrom
vadi2:fix-package-exporter

Conversation

@vadi2
Copy link
Copy Markdown
Member

@vadi2 vadi2 commented Jul 18, 2018

Added a syncronous method for it to use

Brief overview of PR changes/additions

Fix package exporter to work

Motivation for adding to Mudlet

Fix #1790.

Other info (issues closed, discussion etc)

Added a syncronous method for it to use, like it did previously.

vadi2 added 2 commits July 18, 2018 08:15
Added a syncronous method for it to use
@vadi2 vadi2 requested a review from a team as a code owner July 18, 2018 06:17
@vadi2 vadi2 requested a review from a team July 18, 2018 06:17
@vadi2 vadi2 added this to the 3.12 milestone Jul 18, 2018
@SlySven
Copy link
Copy Markdown
Member

SlySven commented Jul 24, 2018

You may want to note as a separate issue the other use of the obsolete zip_add which I see in the Host::saveModules(int, bool) for the current development branch code.

Also, thinking ahead, if that one is also changed, then, given that we are going all international we may need/want to explicitly also use the ZIP_FL_ENC_UTF_8 flag so that non-ASCII file names get interpreted correctly (as UTF-8 rather than the original CP-437 that the Zip specification uses unless flagged otherwise {to that UTF-8})...

@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Jul 24, 2018

Okay, let's do that in another PR to keep the scope here.

//FIXME: report error to userqDebug()<<"zip source error"<<fullName<<fname<<buf;
}
err = zip_add(archive, fname.toStdString().c_str(), s);
err = zip_file_add(archive, fname.toStdString().c_str(), s, ZIP_FL_OVERWRITE);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, I was testing this and found it is still not enough to fix things - it does not handle sub-directories and will silently fail should they be present, (with an "unknown error" when the zip_close(archive) call is made {on line 297}) when all the work is done and finds that it is trying to add a directory and not a file with a zip_file_add call! 🙁

To cure that I suggest you need to make two passes through contents and in the first pass identify the entries that are sub-directories, use zip_dir_add to put them into the archive then make a second pass and identify the remaining files to insert/replace with zip_file_add. You will want to consider what other entries might appear, e.g. symbolic links, junction {or other re-parse} points} and avoid them - which can be done by basically, on line 275 changing:

QStringList contents = dir.entryList();

to:
QStringList contents = dir.entryList(QDir::AllDirs|QDir::Files|QDir::NoSymLinks|QDir::NoDotAndDotDot);

This will filter out the . and .. entries so that lines 278 to 280 then become redundant.

For each entry you will want a QFileInfo instance for it - so it may be more efficient to use QDir::entryInfoList(...) rather than QDir::entryList(...) and to iterate through that - then you can use QFileInfo::isDir() to identify the directories and QFileInfo::isFile() to spot the files. Furthermore given that there could be nested sub-directories you may want to gather them all up and then create them, at the end of the first path, in order of length (shortest first) in the archive so that you do not have a sub-directory before it's parent!

Addressing this will finally mean that sub-directories can be included in the archive as has previously seemed to have been promised by the exporter system - note that the companion code to read sub-directories in mudlet modules has been around for a year or more (see fcb8d64).

@Kebap
Copy link
Copy Markdown
Contributor

Kebap commented Jul 24, 2018

Test results in #1790

@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Jul 24, 2018

Pretty sure that subdirectories are an existing ailing problem - let's get this merged so the PR doesn't scope creep and then we can go on fixing other things.

@vadi2 vadi2 changed the title Fix package exporter to work Fix package exporter to work with async save Jul 25, 2018
@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Jul 25, 2018

I'll merge so there's room to fix the new issues discovered, since the original in #1790 is fixed and the PR is leaking in scope now. I'll address the new issues right after.

Copy link
Copy Markdown
Contributor

@Kebap Kebap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No relevant UI changes found.
Test in #1790 seems successful.

@vadi2 vadi2 merged commit 4c235e3 into Mudlet:development Jul 25, 2018
@vadi2 vadi2 deleted the fix-package-exporter branch July 25, 2018 06:50
SlySven added a commit to SlySven/Mudlet that referenced this pull request Aug 2, 2018
# By Vadim Peretokin (81) and others
* development: (162 commits)
  BugFix: set Server Encoding correctly on auto-loading profiles
  Install xz-utils to guarantee xz is available
  Create and upload source tarballs on release
  BugFix: use Alternative OpenSSLBinary for Windows (Mudlet#1850)
  Fix auto-save kicking in and blocking save profile as
  Upgrade a few classes to newer Qt connect style (Mudlet#1846)
  Fix package exporter to work with async save (Mudlet#1832)
  Pulled out actions into a mudlet member class variables (Mudlet#1839)
  Create module zip if it's not already created when syncing (Mudlet#1842)
  i18n-ise GUI label creation (Mudlet#1838)
  Align "no map"message in centre propely (Mudlet#1837)
  New Crowdin translations (Mudlet#1756)
  Delete old version checks (Mudlet#1833)
  Re-set dev.
  3.11.1 bugfix release.
  Fix iterator to actually iterate
  BugFix: fix faulty log options for new profiles or old profile save files
  Big5 support (Mudlet#1808)
  BugFix: include a fail-back icon for the auto-saved profile in Con. Dialog
  Back to development we go!
  ...

Conflicts resolved in:
* src/TTextEdit.cpp
* src/TTextEdit.h

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit to SlySven/Mudlet that referenced this pull request Jun 22, 2020
# By Vadim Peretokin (24) and others
# Via keneanung
* development: (52 commits)
  Install xz-utils to guarantee xz is available
  Create and upload source tarballs on release
  BugFix: use Alternative OpenSSLBinary for Windows (Mudlet#1850)
  Fix auto-save kicking in and blocking save profile as
  Upgrade a few classes to newer Qt connect style (Mudlet#1846)
  Fix package exporter to work with async save (Mudlet#1832)
  Pulled out actions into a mudlet member class variables (Mudlet#1839)
  Create module zip if it's not already created when syncing (Mudlet#1842)
  i18n-ise GUI label creation (Mudlet#1838)
  Align "no map"message in centre propely (Mudlet#1837)
  New Crowdin translations (Mudlet#1756)
  Delete old version checks (Mudlet#1833)
  Re-set dev.
  3.11.1 bugfix release.
  Fix iterator to actually iterate
  BugFix: fix faulty log options for new profiles or old profile save files
  Big5 support (Mudlet#1808)
  BugFix: include a fail-back icon for the auto-saved profile in Con. Dialog
  Back to development we go!
  3.11.0 release
  ...

Conflicts resolved in:
* src/dlgProfilePreferences.cpp
* src/mudlet.qrc

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
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.

Mudlet Crashing When Using Package Exporter

3 participants