Fix: [smb] Compressed file seek detection #317
Merged
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
-- Compressed file seek detection Log: fix issue Bug: Bug: https://pms.uniontech.com/bug-view-278613.html
Reviewer's GuideThis PR introduces a file‐seek support check for compressed archives by adding a new utility function, integrating early‐fail seek detection into CLI and plugin operations, and extending the UI to surface a dedicated file‐seek error. Sequence diagram for early seek support check in archive operationssequenceDiagram
participant "CliInterface"
participant "Common"
participant "MainWindow"
participant "User"
User->>"CliInterface": Initiate archive operation (list/extract/add)
"CliInterface"->>"Common": isSupportSeek(m_strArchiveName)
"Common"-->>"CliInterface": true/false
alt Seek not supported
"CliInterface"->>"MainWindow": Emit error ET_FileSeekError
"MainWindow"->>"User": Show error message (EI_FileSeekError)
end
Class diagram for updated Common class and error enumsclassDiagram
class Common {
+bool isSupportSeek(QString sFileName)
+bool isSubpathOfDlnfs(const QString &path)
+QString handleLongNameforPath(...)
-bool findDlnfsPath(...)
}
class ErrorType {
ET_UserCancelOpertion
ET_ExistVolume
ET_FileSeekError
}
class ErrorInfo {
EI_InsufficientDiskSpace
EI_ArchiveNoData
EI_ExistVolume
EI_FileSeekError
}
Common --> ErrorType
Common --> ErrorInfo
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来对这段代码进行审查分析：
a) bool Common::isSupportSeek(QString sFileName)
{
QFileInfo info(sFileName);
if(info.exists()) {
// ... 文件存在时的处理
} else {
// 文件不存在时创建临时文件测试
QString tempDir = info.absoluteDir().path();
QString fileTemplate = tempDir + "/tempfile_XXXXXX";
QTemporaryFile tempFile(fileTemplate);
// ...
}
}问题：
建议改进： bool Common::isSupportSeek(const QString &sFileName)
{
// 直接检查目标路径所在的文件系统
QFileInfo info(sFileName);
QString path = info.exists() ? sFileName : info.absolutePath();
QFile file(path);
if (!file.open(QIODevice::ReadOnly)) {
return false;
}
bool canSeek = file.seek(0);
file.close();
return canSeek;
}b) 错误处理不一致： // 在 extractFiles 中
return PFT_Nomral; // 拼写错误且返回值不匹配
// 在其他地方
return PFT_Error; // 正确的错误返回值建议：
c) 代码重复： if (!m_common->isSupportSeek(m_strArchiveName)) {
// 相同的错误处理代码
}建议：
d) 性能优化： QTimer::singleShot(1000, this, [=]() {
// 错误处理
});问题：
建议：
e) 安全性问题：
a) 添加详细的错误日志： qDebug() << "Seek operation not supported for file:" << sFileName;b) 考虑添加缓存机制，避免重复检查同一文件系统 c) 添加单元测试覆盖各种场景：
d) 文档完善：
这些修改将提高代码的健壮性、性能和可维护性。 |
![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> `3rdparty/interface/common.cpp:474` </location>
<code_context>
+ tempFile.flush();
+ }
+ tempFile.close();
+ QString sFileName = tempFile.fileName();
+ QFile file(sFileName);
+ if(file.open(QIODevice::ReadOnly)) {
</code_context>
<issue_to_address>
**nitpick:** Variable shadowing of sFileName may cause confusion.
Consider renaming the inner 'sFileName' variable to avoid shadowing the function parameter and improve code clarity.
</issue_to_address>
### Comment 2
<location> `3rdparty/interface/archiveinterface/cliinterface.cpp:110` </location>
<code_context>
+ emit signalprogress(100);
+ emit signalFinished(PFT_Error);
+ });
+ return PFT_Nomral;
+ }
+
</code_context>
<issue_to_address>
**issue (typo):** Typo in return value: 'PFT_Nomral' should be 'PFT_Normal'.
Please update the return value to 'PFT_Normal' to ensure correctness.
```suggestion
+ return PFT_Normal;
```
</issue_to_address>
### Comment 3
<location> `3rdparty/interface/common.cpp:449` </location>
<code_context>
});
}
+bool Common::isSupportSeek(QString sFileName)
+{
+ QFileInfo info(sFileName);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the file open/seek logic into a helper function to eliminate duplication and clarify intent.
```suggestion
Extract the “open-and-seek” logic into a small helper, then collapse the two branches into a single flow. For example:
```cpp
// Common.h
class Common {
…
static bool trySeekFile(const QString& path);
bool isSupportSeek(const QString& sFileName);
};
// Common.cpp
bool Common::trySeekFile(const QString& path)
{
QFile file(path);
if (!file.open(QIODevice::ReadOnly))
return false;
bool ok = file.seek(0);
file.close();
return ok;
}
bool Common::isSupportSeek(const QString& sFileName)
{
QFileInfo info(sFileName);
if (info.exists()) {
return trySeekFile(sFileName);
}
// Create a temp file in the same directory
QString templatePath = info.absoluteDir().filePath("tempfile_XXXXXX");
QTemporaryFile tempFile(templatePath);
tempFile.setAutoRemove(true);
if (!tempFile.open())
return false;
tempFile.write("test\n");
tempFile.close();
return trySeekFile(tempFile.fileName());
}
```
Benefits:
- No duplicate open/seek/close
- Single return points for each branch
- Clear intent in `trySeekFile`
```
</issue_to_address>
### Comment 4
<location> `3rdparty/interface/archiveinterface/cliinterface.cpp:78` </location>
<code_context>
m_workStatus = WT_List;
+ // 是否支持seek
+ if(!m_common->isSupportSeek(m_strArchiveName)) {
+ QTimer::singleShot(1000, this, [=]() {
+ m_eErrorType = ET_FileSeekError;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the repeated QTimer error-handling code into a helper function and macro to reduce duplication.
Here’s one way to collapse those three identical QTimer blocks into a single helper + guard macro. This preserves your exact return-values but only writes the “error timer” code once.
```cpp
// In CliInterface.h (or a common private section)
private:
void emitSeekErrorAndFinish() {
QTimer::singleShot(1000, this, [this]() {
m_eErrorType = ET_FileSeekError;
emit signalprogress(100);
emit signalFinished(PFT_Error);
});
}
#define RETURN_IF_NO_SEEK(retval) \
do { \
if (!m_common->isSupportSeek(m_strArchiveName)) { \
emitSeekErrorAndFinish(); \
return retval; \
} \
} while (0)
```
Then your three methods collapse to:
```cpp
PluginFinishType CliInterface::list() {
setPassword(QString());
DataManager::get_instance().resetArchiveData();
m_setHasRootDirs.clear();
m_setHasHandlesDirs.clear();
m_workStatus = WT_List;
RETURN_IF_NO_SEEK(PFT_Error);
bool ret = runProcess( /* … */ );
return ret ? PFT_Nomral : PFT_Error;
}
PluginFinishType CliInterface::extractFiles(...) {
RETURN_IF_NO_SEEK(PFT_Nomral);
// … rest of extractFiles …
}
PluginFinishType CliInterface::addFiles(...) {
// only guard when files not empty
if (!files.isEmpty()) {
RETURN_IF_NO_SEEK(PFT_Nomral);
}
// … rest of addFiles …
}
```
That removes three copies of the QTimer+lambda while keeping every return‐value exactly as before.
</issue_to_address>
### Comment 5
<location> `src/source/mainwindow.cpp:1812` </location>
<code_context>
showErrorMessage(FI_Compress, EI_ExistVolume, true);
break;
+ // ftp目录不支持seek操作
+ case ET_FileSeekError:
+ showErrorMessage(FI_Compress, EI_FileSeekError, true);
+ break;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring repeated ET_FileSeekError/EI_FileSeekError handling into helper functions to avoid code duplication.
```markdown
You’re repeating the same ET_FileSeekError/EI_FileSeekError handling in every switch. You can pull that logic out into one helper per subsystem and early‐return, or at least factor the UI messages into a lookup—then each switch only needs:
1) one `if (eErrorType == ET_FileSeekError) { … }`
2) a normal switch without the duplicated case
Example for the job‐level error→popup section:
```cpp
// new helper at top of file:
bool handleSeekError(ArchiveJob::JobType jt, ErrorType e, bool needsConfirm)
{
if (e != ET_FileSeekError)
return false;
// map job → FI_*
FileInterface fi;
switch (jt) {
case ArchiveJob::JT_Compress: fi = FI_Compress; break;
case ArchiveJob::JT_Load: fi = FI_Load; break;
case ArchiveJob::JT_Extract:
case ArchiveJob::JT_StepExtract: fi = FI_Uncompress; break;
default: fi = FI_Compress; /* fallback */ }
showErrorMessage(fi, EI_FileSeekError, needsConfirm);
return true;
}
// in your outer job switch:
case ArchiveJob::JT_Compress: {
bool needsConfirm = true; // existing logic
if (handleSeekError(m_eJobType, eErrorType, needsConfirm))
break;
switch (eErrorType) {
// … now no ET_FileSeekError here …
}
} break;
```
And for the UI‐mapping you can centralize the detail strings:
```cpp
// helper in .cpp
static QString seekDetail(FileInterface fi) {
switch (fi) {
case FI_Compress:
return tr("No compression support in current directory. Download the files to a local device.");
case FI_Load:
return tr("Can't open compressed packages in current directory. Download the compressed package to a local device.");
case FI_Uncompress:
return tr("No extraction support in current directory. Download the compressed package to a local device.");
default:
return tr("Operation not supported in current directory.");
}
}
// then in your failure‐page switch:
case EI_FileSeekError:
m_pFailurePage->setFailureDetail(seekDetail(currentFI));
break;
```
These two small helpers remove all the repetitive `case ET_FileSeekError` and `case EI_FileSeekError` blocks while keeping your behavior identical.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| tempFile.flush(); | ||
| } | ||
| tempFile.close(); | ||
| QString sFileName = tempFile.fileName(); |
There was a problem hiding this comment.
nitpick: Variable shadowing of sFileName may cause confusion.
Consider renaming the inner 'sFileName' variable to avoid shadowing the function parameter and improve code clarity.
| emit signalprogress(100); | ||
| emit signalFinished(PFT_Error); | ||
| }); | ||
| return PFT_Nomral; |
There was a problem hiding this comment.
issue (typo): Typo in return value: 'PFT_Nomral' should be 'PFT_Normal'.
Please update the return value to 'PFT_Normal' to ensure correctness.
Suggested change
| return PFT_Nomral; | |
| + return PFT_Normal; |
| m_workStatus = WT_List; | ||
|
|
||
| // 是否支持seek | ||
| if(!m_common->isSupportSeek(m_strArchiveName)) { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the repeated QTimer error-handling code into a helper function and macro to reduce duplication.
Here’s one way to collapse those three identical QTimer blocks into a single helper + guard macro. This preserves your exact return-values but only writes the “error timer” code once.
// In CliInterface.h (or a common private section)
private:
void emitSeekErrorAndFinish() {
QTimer::singleShot(1000, this, [this]() {
m_eErrorType = ET_FileSeekError;
emit signalprogress(100);
emit signalFinished(PFT_Error);
});
}
#define RETURN_IF_NO_SEEK(retval) \
do { \
if (!m_common->isSupportSeek(m_strArchiveName)) { \
emitSeekErrorAndFinish(); \
return retval; \
} \
} while (0)Then your three methods collapse to:
PluginFinishType CliInterface::list() {
setPassword(QString());
DataManager::get_instance().resetArchiveData();
m_setHasRootDirs.clear();
m_setHasHandlesDirs.clear();
m_workStatus = WT_List;
RETURN_IF_NO_SEEK(PFT_Error);
bool ret = runProcess( /* … */ );
return ret ? PFT_Nomral : PFT_Error;
}
PluginFinishType CliInterface::extractFiles(...) {
RETURN_IF_NO_SEEK(PFT_Nomral);
// … rest of extractFiles …
}
PluginFinishType CliInterface::addFiles(...) {
// only guard when files not empty
if (!files.isEmpty()) {
RETURN_IF_NO_SEEK(PFT_Nomral);
}
// … rest of addFiles …
}That removes three copies of the QTimer+lambda while keeping every return‐value exactly as before.
lzwind
approved these changes
Nov 20, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GongHeng2017, lzwind 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 |
Author
|
/merge |
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.
-- Compressed file seek detection
Log: fix issue
Bug: Bug: https://pms.uniontech.com/bug-view-278613.html
Summary by Sourcery
Implement filesystem seek support detection and gracefully handle cases where seek is not supported during archive operations
Bug Fixes:
Enhancements: