-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support macOS promise file handling #6458
Support macOS promise file handling #6458
Conversation
@@ -26,6 +28,12 @@ class IMPORT_EXPORT_API ImportProgressListener | |||
}; | |||
|
|||
virtual ~ImportProgressListener(); | |||
|
|||
/// Set of functions called by importer before getting file handle as it may take a while to process | |||
/// when dealing with file promise on macOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think doxygen-style comment will be recognized as comment to the first function only, not a whole group. Should be single-line C-style comment //
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ImportInfoDialog.h
Outdated
private: | ||
void OnCancel(wxCommandEvent& event); | ||
|
||
wxStaticText* m_statusText; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't mix code-style in a single header, we use camel-case prefixed with m
for member variable names (except for POD structures)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ImportInfoDialog.h
Outdated
@@ -0,0 +1,25 @@ | |||
#pragma once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File header comment is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ImportInfoDialog.h
Outdated
#include <wx/wx.h> | ||
#include "wxPanelWrapper.h" | ||
|
||
class ImportInfoDialog : public wxDialogWrapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ProjectFileManager.cpp
Outdated
@@ -1306,6 +1308,26 @@ class ImportProgress final | |||
|
|||
} | |||
|
|||
void OnImportStarted(const wxString &filename) override | |||
{ | |||
mImportInfoDialog = new ImportInfoDialog(filename, NULL, -1, XO("Importing files")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dialog will appear on every import, even for tiny files that are readable, won't it be too intrusive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a way to determine if file is already readable or in the middle of download so I guess it needs to stay that way.
There's other option though: there is a progress dialog displayed right after obtaining file handle - instead of showing ImportInfo dialog and then ProgressDialog, we could display only ProgressDialog but sooner - but this needs some refactoring because ProgressDialog expects file handle in its constructor.
Right now it looks like ImportInfo dialog "evolves" into ProgressDialog
std::future<std::unique_ptr<ImportFileHandle>> future = promise->get_future(); | ||
std::thread([promise, plugin, fName, pProj]() mutable { | ||
try { | ||
auto result = plugin->Open(fName, pProj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you make sure that all calls are thread-safe? Isn't it enough to try simply fopen
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried it, unfortunately fopen returns false-positive true and after that when you call plugin->Open UI freezes anyway.
I think this is the only option we have. The most vulnerable scenario is when user cancels importing because thread is working until it finishes it's job and by that time we're out of scope so I've used shared_ptr to take care of promise object to be still available (avoiding potential segfault that way)
src/ImportInfoDialog.cpp
Outdated
{ | ||
wxBoxSizer* sizer = new wxBoxSizer(wxVERTICAL); | ||
|
||
m_statusText = new wxStaticText(this, wxID_ANY, wxString::Format(wxT("Importing %s..."), filename.AfterLast(wxFileName::GetPathSeparator()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use either XO
or _
macro for translatable strings, otherwise gettext
will not see your string definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ImportInfoDialog.cpp
Outdated
else | ||
CenterOnScreen(); | ||
|
||
Bind(wxEVT_BUTTON, &ImportInfoDialog::OnCancel, this, wxID_CANCEL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handler is already bound to the event via event table you've defined above, one of them should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ProjectFileManager.cpp
Outdated
wxYield(); | ||
} | ||
|
||
bool OnCheckImportCancelled() override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't be called after OnImportStartCleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/ProjectFileManager.cpp
Outdated
return mImportInfoDialog->IsCancelled(); | ||
} | ||
|
||
void OnImportStartCleanup() override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't need to be a part of interface. Please consider using ProjectWindows
or assigning this dialog directly to a project frame via GetProjectFrame
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assigned dialog to project frame but left function and renamed it (still need to call EndModal)
0bf2c03
to
0e54b3c
Compare
0e54b3c
to
93ccc0b
Compare
libraries/lib-basic-ui/BasicUI.h
Outdated
@@ -311,10 +312,10 @@ inline std::unique_ptr<ProgressDialog> MakeProgress( | |||
*/ | |||
inline std::unique_ptr<GenericProgressDialog> MakeGenericProgress( | |||
const WindowPlacement &placement, | |||
const TranslatableString &title, const TranslatableString &message) | |||
const TranslatableString &title, const TranslatableString &message, int style = 0x0002 | 0x0008 | 0x0020) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing: lib-basic-ui
is a toolkit-neutral library, better define enum
and map it to a framework-specific value in the library that implements BasicUI
interface, which is lib-wx-init
93ccc0b
to
96622fb
Compare
if(err != 0 || (S_ISREG(s.st_mode) && s.st_blocks == 0)) | ||
{ | ||
int dialogStyle = BasicUI::ProgressCanAbort | BasicUI::ProgressAppModal | BasicUI::ProgressShowElapsedTime | BasicUI::ProgressSmooth; | ||
auto dialog = BasicUI::MakeGenericProgress({}, XO("Audacity"), XO("This may take several seconds"), dialogStyle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be XO("Importing files"), XO("Importing %s...")
where %s is the file that currently is being imported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 👍
96622fb
to
7b72974
Compare
@grliszas14 for a single-commit PR's please use "rebase and merge", that makes history look much clearer. Ideally before merging a branch with multiple commits rebase your branch on top of recent master first, wait checks are complete and then do "merge commit" |
Resolves: #6376
Getting file handle of file that hasn't been downloaded yet (in the middle of download) is a blocking call. If file is big or internet connection is slow this call freezes whole app.
Moved blocking call to a separate thread and allowed user to cancel import (if so, user goes back to usual flow, thread is going to finish it's job but the result is ignored, thread cleans up after itself)
Recommended: