Support macOS promise file handling #6458

grliszas14 · 2024-05-20T08:19:03Z

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)

I signed CLA
The title of the pull request describes an issue it addresses
If changes are extensive, then there is a sequence of easily reviewable commits
Each commit's message describes its purpose and effects
There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

Each commit compiles and runs on my machine without known undesirable changes of behavior

vsverchinsky · 2024-05-21T08:01:05Z

libraries/lib-import-export/ImportProgressListener.h

@@ -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


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 //

vsverchinsky · 2024-05-21T08:03:06Z

src/ImportInfoDialog.h

+private:
+    void OnCancel(wxCommandEvent& event);
+
+    wxStaticText* m_statusText;


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)

vsverchinsky · 2024-05-21T08:04:38Z

src/ImportInfoDialog.h

@@ -0,0 +1,25 @@
+#pragma once


File header comment is missing

vsverchinsky · 2024-05-21T08:04:51Z

src/ImportInfoDialog.h

+#include <wx/wx.h>
+#include "wxPanelWrapper.h"
+
+class ImportInfoDialog : public wxDialogWrapper


vsverchinsky · 2024-05-21T08:08:29Z

src/ProjectFileManager.cpp

@@ -1306,6 +1308,26 @@ class ImportProgress final

   }

+   void OnImportStarted(const wxString &filename) override
+   {
+       mImportInfoDialog = new ImportInfoDialog(filename, NULL, -1, XO("Importing files"));


This dialog will appear on every import, even for tiny files that are readable, won't it be too intrusive?

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

vsverchinsky · 2024-05-21T08:16:20Z

libraries/lib-import-export/Import.cpp

+      std::future<std::unique_ptr<ImportFileHandle>> future = promise->get_future();
+      std::thread([promise, plugin, fName, pProj]() mutable {
+         try {
+            auto result = plugin->Open(fName, pProj);


Did you make sure that all calls are thread-safe? Isn't it enough to try simply fopen file?

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)

vsverchinsky · 2024-05-21T08:20:17Z

src/ImportInfoDialog.cpp

+{
+    wxBoxSizer* sizer = new wxBoxSizer(wxVERTICAL);
+
+    m_statusText = new wxStaticText(this, wxID_ANY, wxString::Format(wxT("Importing %s..."), filename.AfterLast(wxFileName::GetPathSeparator())));


Use either XO or _ macro for translatable strings, otherwise gettext will not see your string definition

vsverchinsky · 2024-05-21T08:22:33Z

src/ImportInfoDialog.cpp

+    else
+       CenterOnScreen();
+
+    Bind(wxEVT_BUTTON, &ImportInfoDialog::OnCancel, this, wxID_CANCEL);


Handler is already bound to the event via event table you've defined above, one of them should be removed

vsverchinsky · 2024-05-21T08:25:25Z

src/ProjectFileManager.cpp

+       wxYield();
+   }
+
+   bool OnCheckImportCancelled() override


Can't be called after OnImportStartCleanup

vsverchinsky · 2024-05-21T08:30:43Z

src/ProjectFileManager.cpp

+        return mImportInfoDialog->IsCancelled();
+   }
+
+   void OnImportStartCleanup() override


Doesn't need to be a part of interface. Please consider using ProjectWindows or assigning this dialog directly to a project frame via GetProjectFrame

assigned dialog to project frame but left function and renamed it (still need to call EndModal)

vsverchinsky · 2024-05-24T13:03:39Z

libraries/lib-basic-ui/BasicUI.h

@@ -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)


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

LWinterberg · 2024-05-27T12:19:09Z

libraries/lib-import-export/Import.cpp

+      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);


This should be XO("Importing files"), XO("Importing %s...") where %s is the file that currently is being imported

vsverchinsky · 2024-05-28T17:45:20Z

@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"

vsverchinsky reviewed May 21, 2024

View reviewed changes

grliszas14 force-pushed the copy_paste_improvements branch 3 times, most recently from 0bf2c03 to 0e54b3c Compare May 24, 2024 08:16

vsverchinsky approved these changes May 24, 2024

View reviewed changes

grliszas14 force-pushed the copy_paste_improvements branch from 0e54b3c to 93ccc0b Compare May 24, 2024 10:44

vsverchinsky reviewed May 24, 2024

View reviewed changes

vsverchinsky self-requested a review May 24, 2024 13:05

grliszas14 force-pushed the copy_paste_improvements branch from 93ccc0b to 96622fb Compare May 27, 2024 06:05

vsverchinsky approved these changes May 27, 2024

View reviewed changes

LWinterberg requested changes May 27, 2024

View reviewed changes

Support macOS promise file handling

7b72974

grliszas14 force-pushed the copy_paste_improvements branch from 96622fb to 7b72974 Compare May 27, 2024 12:58

vsverchinsky requested a review from LWinterberg May 27, 2024 12:59

LWinterberg approved these changes May 27, 2024

View reviewed changes

grliszas14 merged commit 17a0b53 into audacity:master May 28, 2024
10 checks passed

grliszas14 deleted the copy_paste_improvements branch May 28, 2024 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support macOS promise file handling #6458

Support macOS promise file handling #6458

grliszas14 commented May 20, 2024

vsverchinsky May 21, 2024

grliszas14 May 21, 2024

vsverchinsky May 21, 2024

grliszas14 May 21, 2024

vsverchinsky May 21, 2024

grliszas14 May 21, 2024

vsverchinsky May 21, 2024

grliszas14 May 21, 2024

vsverchinsky May 21, 2024 •

edited

grliszas14 May 21, 2024

vsverchinsky May 21, 2024 •

edited

grliszas14 May 21, 2024

vsverchinsky May 21, 2024

grliszas14 May 21, 2024

vsverchinsky May 21, 2024

grliszas14 May 21, 2024

vsverchinsky May 21, 2024

vsverchinsky May 21, 2024

grliszas14 May 21, 2024

vsverchinsky May 21, 2024

grliszas14 May 21, 2024

vsverchinsky May 24, 2024

LWinterberg May 27, 2024

grliszas14 May 27, 2024

vsverchinsky commented May 28, 2024

Support macOS promise file handling #6458

Support macOS promise file handling #6458

Conversation

grliszas14 commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsverchinsky May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsverchinsky May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsverchinsky commented May 28, 2024

vsverchinsky May 21, 2024 •

edited

vsverchinsky May 21, 2024 •

edited