fix: keep renamed file content during compression #312
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- copy aliased entries into a temp directory before invoking createArchive - update FileEntry paths to point at the real copies and clear aliases - cleanup the temp copies on success, failure, cancel, and reset - ensures libzip sees actual files instead of symlink paths Log: fix bug Bug: https://pms.uniontech.com/bug-view-340081.html
Reviewer's GuideAdds a temp‐copy workflow for aliased entries so compression always operates on real files: introduces a recursive directory copy helper, prepare/cleanup routines for alias entries, updates FileEntry paths, and hooks these routines into the compression success, failure, cancel, and reset flows. Sequence diagram for compression workflow with alias file preparation and cleanupsequenceDiagram
participant User
participant MainWindow
participant CompressPage
participant FileSystem
participant ArchiveJob
User->>MainWindow: Initiate compression
MainWindow->>CompressPage: getEntrys()
CompressPage-->>MainWindow: Return listEntry
MainWindow->>MainWindow: prepareCompressAliasEntries(listEntry)
MainWindow->>FileSystem: Copy aliased files/directories to temp
FileSystem-->>MainWindow: Temp files/directories created
MainWindow->>ArchiveJob: Start compression with updated paths
ArchiveJob-->>MainWindow: Compression result (success/failure/cancel)
MainWindow->>MainWindow: cleanupCompressAliasEntries()
MainWindow->>FileSystem: Remove temp files/directories
FileSystem-->>MainWindow: Temp cleaned up
Class diagram for MainWindow and FileEntry changesclassDiagram
class MainWindow {
+QString m_strCompressAliasRoot
+bool m_needCleanupCompressAlias
+bool prepareCompressAliasEntries(QList<FileEntry>&)
+void cleanupCompressAliasEntries()
}
class FileEntry {
+QString strFullPath
+QString strAlias
+bool isDirectory
}
MainWindow "1" -- "*" FileEntry : manages
Flow diagram for alias file preparation and cleanup during compressionflowchart TD
A["Start Compression"] --> B["Get FileEntry list"]
B --> C{"Any entries with alias?"}
C -- Yes --> D["Create temp directory"]
D --> E["Copy aliased files/directories to temp"]
E --> F["Update FileEntry paths, clear alias"]
F --> G["Proceed with compression"]
G --> H{"Compression finished (success/failure/cancel/reset)?"}
H -- Yes --> I["Cleanup temp alias files"]
C -- No --> G
I --> J["End"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来对这个代码变更进行详细审查：
改进建议： namespace {
bool copyDirectoryRecursively(const QString &sourcePath, const QString &targetPath)
{
QDir sourceDir(sourcePath);
if (!sourceDir.exists()) {
return false;
}
// 使用QFileInfo处理路径，确保跨平台兼容性
QFileInfo targetInfo(targetPath);
QString targetPathClean = targetInfo.absoluteFilePath();
if (!QDir().mkpath(targetPathClean)) {
return false;
}
const QFileInfoList entries = sourceDir.entryInfoList(
QDir::NoDotAndDotDot | QDir::AllDirs | QDir::Files | QDir::Hidden | QDir::System);
for (const QFileInfo &info : entries) {
QString srcFilePath = info.absoluteFilePath();
QString dstFilePath = targetPathClean + QDir::separator() + info.fileName();
if (info.isDir()) {
if (!copyDirectoryRecursively(srcFilePath, dstFilePath)) {
return false;
}
} else {
// 检查目标文件是否存在，存在则先删除
QFile::remove(dstFilePath);
if (!QFile::copy(srcFilePath, dstFilePath)) {
return false;
}
}
}
return true;
}
// 抽取错误处理函数
bool handleCopyError(const QString &aliasName, const QString &aliasRoot)
{
showWarningDialog(tr("Failed to prepare renamed item \"%1\" for compression, please check permissions and available space.")
.arg(aliasName));
if (!aliasRoot.isEmpty()) {
QDir(aliasRoot).removeRecursively();
}
return false;
}
}
bool MainWindow::prepareCompressAliasEntries(QList<FileEntry> &listEntry)
{
m_needCleanupCompressAlias = false;
m_strCompressAliasRoot.clear();
bool hasAlias = false;
QString aliasRoot;
// 使用更安全的临时目录生成方式
QString baseDir = QStandardPaths::writableLocation(QStandardPaths::TempLocation);
baseDir += QDir::separator() + m_strProcessID + QDir::separator() + "compress_alias_" +
QUuid::createUuid().toString(QUuid::WithoutBraces);
if (!QDir().mkpath(baseDir)) {
showWarningDialog(tr("Failed to create temporary directory, please check and try again."));
return false;
}
for (FileEntry &entry : listEntry) {
if (entry.strAlias.isEmpty() || entry.strFullPath.isEmpty()) {
continue;
}
const QString aliasName = entry.strAlias;
const QString targetPath = baseDir + QDir::separator() + aliasName;
if (entry.isDirectory) {
QDir(targetPath).removeRecursively();
if (!copyDirectoryRecursively(entry.strFullPath, targetPath)) {
return handleCopyError(aliasName, baseDir);
}
} else {
QFile::remove(targetPath);
if (!QFile::copy(entry.strFullPath, targetPath)) {
return handleCopyError(aliasName, baseDir);
}
}
entry.strFullPath = targetPath;
entry.strAlias.clear();
hasAlias = true;
}
if (hasAlias) {
m_strCompressAliasRoot = baseDir;
m_needCleanupCompressAlias = true;
} else {
QDir(baseDir).removeRecursively(); // 清理未使用的临时目录
}
return true;
}
void MainWindow::cleanupCompressAliasEntries()
{
if (m_strCompressAliasRoot.isEmpty()) {
m_needCleanupCompressAlias = false;
return;
}
QDir aliasRoot(m_strCompressAliasRoot);
if (aliasRoot.exists()) {
aliasRoot.removeRecursively();
}
m_strCompressAliasRoot.clear();
m_needCleanupCompressAlias = false;
}主要改进点：
这些改进提高了代码的可维护性、安全性和跨平台兼容性。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/source/mainwindow.cpp:2178-2179` </location>
<code_context>
+ return false;
+ }
+ } else {
+ QFile::remove(targetPath);
+ if (!QFile::copy(entry.strFullPath, targetPath)) {
+ showWarningDialog(tr("Failed to prepare renamed item \"%1\" for compression, please check permissions and available space.")
+ .arg(aliasName));
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unconditionally removing targetPath may cause issues if the file is in use.
If QFile::remove fails due to the file being in use, the subsequent copy may also fail. Please handle the return value of QFile::remove to ensure robust error handling.
</issue_to_address>
### Comment 2
<location> `src/source/mainwindow.cpp:2213-2214` </location>
<code_context>
+ }
+
+ QDir aliasRoot(m_strCompressAliasRoot);
+ if (aliasRoot.exists()) {
+ aliasRoot.removeRecursively();
+ }
+ m_strCompressAliasRoot.clear();
</code_context>
<issue_to_address>
**issue (bug_risk):** removeRecursively may fail silently if files are locked or permissions are insufficient.
Currently, failures from removeRecursively are ignored, which can result in orphaned files. Please check its return value and handle errors to ensure proper cleanup.
</issue_to_address>
### Comment 3
<location> `src/source/mainwindow.cpp:79` </location>
<code_context>
+ return false;
+ }
+
+ const QFileInfoList entries = sourceDir.entryInfoList(QDir::NoDotAndDotDot | QDir::AllDirs | QDir::Files | QDir::Hidden | QDir::System);
+ for (const QFileInfo &info : entries) {
+ const QString srcFilePath = info.absoluteFilePath();
</code_context>
<issue_to_address>
**🚨 question (security):** Including QDir::Hidden and QDir::System may copy unintended files.
Review whether you need to include hidden and system files; if not, remove these flags to avoid potential security or privacy issues.
</issue_to_address>
### Comment 4
<location> `src/source/mainwindow.cpp:2134` </location>
<code_context>
}
}
+bool MainWindow::prepareCompressAliasEntries(QList<FileEntry> &listEntry)
+{
+ m_needCleanupCompressAlias = false;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring all alias preparation and cleanup logic into a dedicated CompressAliasManager class to simplify MainWindow and encapsulate file operations.
Here’s a sketch of how you can pull _all_ of the alias-preparation/copy/cleanup logic out of MainWindow into its own class. This not only hides all the QDir/QFile details but also collapses dozens of lines in MainWindow down to two calls.
1) Create `CompressAliasManager`:
```cpp
// CompressAliasManager.h
#pragma once
#include <QString>
#include <QList>
#include "FileEntry.h" // wherever FileEntry is defined
class CompressAliasManager
{
public:
CompressAliasManager(const QString &tempRootBase, const QString &pid);
~CompressAliasManager();
// return false on error (dialog already shown), in which case no cleanup needed
bool prepare(QList<FileEntry> &entries);
// always safe to call (will only remove if it was created)
void cleanup();
private:
bool copyRecursive(const QString &src, const QString &dst);
QString m_aliasRoot;
bool m_needsCleanup = false;
QString m_tempRootBase;
QString m_pid;
};
```
```cpp
// CompressAliasManager.cpp
#include "CompressAliasManager.h"
#include <QDir>
#include <QFile>
#include <QUuid>
#include <QMessageBox>
CompressAliasManager::CompressAliasManager(const QString &tempRootBase,
const QString &pid)
: m_tempRootBase(tempRootBase), m_pid(pid)
{}
CompressAliasManager::~CompressAliasManager()
{
cleanup();
}
bool CompressAliasManager::prepare(QList<FileEntry> &entries)
{
QStringList toAlias;
for (auto &e : entries) {
if (!e.strAlias.isEmpty() && !e.strFullPath.isEmpty())
toAlias << e.strAlias;
}
if (toAlias.isEmpty())
return true; // nothing to do
// build aliasRoot
QString base = m_tempRootBase + QDir::separator() + m_pid + QDir::separator() + "compress_alias";
if (!QDir().mkpath(base))
return false;
m_aliasRoot = base + QDir::separator() + QUuid::createUuid().toString(QUuid::WithoutBraces);
if (!QDir().mkpath(m_aliasRoot))
return false;
// copy each entry under aliasRoot
for (auto &e : entries) {
if (e.strAlias.isEmpty()) continue;
QString dst = m_aliasRoot + QDir::separator() + e.strAlias;
if (e.isDirectory) {
if (!copyRecursive(e.strFullPath, dst)) {
QMessageBox::warning(nullptr, tr("Error"),
tr("Failed to prepare renamed item \"%1\" for compression.").arg(e.strAlias));
QDir(m_aliasRoot).removeRecursively();
return false;
}
} else {
QFile::remove(dst);
if (!QFile::copy(e.strFullPath, dst)) {
QMessageBox::warning(nullptr, tr("Error"),
tr("Failed to prepare renamed item \"%1\" for compression.").arg(e.strAlias));
QDir(m_aliasRoot).removeRecursively();
return false;
}
}
e.strFullPath = dst;
e.strAlias.clear();
}
m_needsCleanup = true;
return true;
}
void CompressAliasManager::cleanup()
{
if (m_needsCleanup && !m_aliasRoot.isEmpty())
QDir(m_aliasRoot).removeRecursively();
m_needsCleanup = false;
}
bool CompressAliasManager::copyRecursive(const QString &src, const QString &dst)
{
QDir s(src);
if (!s.exists()) return false;
if (!QDir().mkpath(dst)) return false;
for (auto &info : s.entryInfoList(QDir::NoDotAndDotDot |
QDir::AllDirs | QDir::Files |
QDir::Hidden | QDir::System)) {
QString childDst = dst + QDir::separator() + info.fileName();
if (info.isDir()) {
if (!copyRecursive(info.absoluteFilePath(), childDst))
return false;
} else {
QFile::remove(childDst);
if (!QFile::copy(info.absoluteFilePath(), childDst))
return false;
}
}
return true;
}
```
2) In `MainWindow` replace all of the in-class free functions + member flags with:
```cpp
#include "CompressAliasManager.h"
class MainWindow : public DMainWindow {
// ...
std::unique_ptr<CompressAliasManager> m_aliasMgr;
// ...
}
MainWindow::MainWindow(...) {
// after you compute PID:
m_aliasMgr.reset(new CompressAliasManager(TEMPPATH, m_strProcessID));
}
// in your slotCompress:
if (!m_aliasMgr->prepare(listEntry))
return;
// ...
// on error / in every slotJobFinished / resetMainWindow:
m_aliasMgr->cleanup();
```
Advantages:
- MainWindow.cpp shrinks by several hundred lines.
- All QDir/QFile details live in one focused class.
- Cleanup/RAII guarantees no temp-dir leaks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+2178
to
+2179
| QFile::remove(targetPath); | ||
| if (!QFile::copy(entry.strFullPath, targetPath)) { |
There was a problem hiding this comment.
suggestion (bug_risk): Unconditionally removing targetPath may cause issues if the file is in use.
If QFile::remove fails due to the file being in use, the subsequent copy may also fail. Please handle the return value of QFile::remove to ensure robust error handling.
max-lvs
approved these changes
Nov 11, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiHua000, max-lvs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/forcemerge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Log: fix bug
Bug: https://pms.uniontech.com/bug-view-340081.html
Summary by Sourcery
Fix compression of renamed files by copying aliased entries into a temporary directory, updating their paths for archiving, and cleaning up afterwards.
Bug Fixes:
Enhancements: