Fix: Prevent UI reset when new device inserted during write (#825) #914

Prachi194agrawal · 2025-12-16T04:27:02Z

When a new USB drive was inserted while another drive was being written to, the drive selection would automatically change to the newly inserted drive, causing the progress bar to disappear. The write process continued in the background, but users had no visual feedback.

This fix checks if the currently selected drive has an active write operation (progress > 0 and < drive size) before changing the selection. If a write is in progress, the selection remains unchanged when new drives are connected, preserving the progress display.

Fixes #825

…#825) When a new USB drive was inserted while another drive was being written to, the drive selection would automatically change to the newly inserted drive, causing the progress bar to disappear. The write process continued in the background, but users had no visual feedback. This fix checks if the currently selected drive has an active write operation (progress > 0 and < drive size) before changing the selection. If a write is in progress, the selection remains unchanged when new drives are connected, preserving the progress display. Fixes FedoraQt#825 Signed-off-by: Prachi194agrawal <[email protected]>

grulja · 2025-12-22T13:10:16Z

Hi, thank you for your change. While I believe it fixes the issue, I have a slightly different solution in mind that I would like to apply instead, but it might require a bigger change. I will try to look into this as soon as possible.

Prachi194agrawal · 2025-12-25T16:17:07Z

Hi @grulja, thanks for the feedback!

I understand that you have a different approach in mind which may involve a larger change. If you have a moment, could you briefly explain the direction you’re considering (even at a high level)? That would help me understand the reasoning and see if I can either adjust my current fix or help work on the alternative solution.

Thanks again for taking a look!

grulja · 2026-01-09T14:06:10Z

Without further blocking this, I think there are two possible solutions:

Either we stop changing the device selection once we insert a new device. I think this was done based on the assumption that the user will always want to automatically pick the newly inserted device. Instead I would only do just update of m_selectedIndex based on whether the new device is inserted to the list before the already selected device so you would just increment m_selectedIndex to still point to the same device.
Maybe a better approach would be to implement something like Drive::isBusy() that would track the state of the device. Since each operation (write or restore) creates and starts a QProcess and ends with onFinished() callback, you get the information when the operation ends. With that you can have something like:

// Moved up from the end of the function
if (d->restoreStatus() == Drive::CONTAINS_LIVE) {
    setLastRestoreable(d);
    connect(d, &Drive::restoreStatusChanged, [=]() {
        if (d && d == m_lastRestoreable && d->restoreStatus() != Drive::CONTAINS_LIVE)
            setLastRestoreable(nullptr);
    });
 } 

if (selected()->isBusy()) {
    return;
}

if (m_provider->initialized()) {
    m_selectedIndex = position;
    emit selectedChanged();
} else {
    m_selectedIndex = 0;
    emit selectedChanged();
}

What do you think? @gastoner any opinion?

gastoner · 2026-01-09T19:28:45Z

I would prefer the v2, liking the solution with isBusy function. This is probably easier to understand too, compared to switching indexes while being in some state etc...

grulja

Please implement the changes as requested in the comment above. Thank you.

…raQt#825) Replace progress-based logic with a cleaner state-based approach by adding an isBusy() method to the Drive class. This encapsulates the busy state within the Drive object itself, making the code more maintainable. Changes: - Add isBusy() getter method and m_isBusy member variable to Drive class - Set m_isBusy = true when write() or restore() operations start - Reset m_isBusy = false when operations complete in onFinished() callbacks - Update DriveManager::onDriveConnected() to check isBusy() instead of progress - Apply changes across all platform implementations (Linux, Mac, Windows) This prevents the UI from resetting when a new USB device is inserted during an active write or restore operation. Fixes FedoraQt#825 Signed-off-by: Prachi194agrawal <[email protected]>

grulja

Each implementation [Linux/Mac/Windows]DriveManager has QProcess as a member variable. Can we possibly move this to the Drive itself and remove it in each implementation so it's a common member for all of them. Then you can check the state of the process instead of tracking the state yourself through m_isBusy.

Btw. it looks WinDriveManager already has bool WinDrive::busy() implementation doing exactly that so re-use that and just move it to the common class so all the other implementations have this as well.

- Moved QProcess *m_process to Drive base class (shared by all platforms) - Removed manual m_isBusy boolean flag entirely - Implemented isBusy() to check actual QProcess::state() - Updated all platform implementations (Linux/Mac/Windows) to use inherited m_process - Renamed m_child to m_process in Mac and Windows implementations - Removed all manual busy flag assignments This provides a cleaner, state-based architecture where busy status is determined by the actual process state rather than manual tracking.

Prachi194agrawal · 2026-01-14T18:56:43Z

Sir,
I've implemented the requested changes in commit 6d63fbc:
Moved QProcess *m_process to the Drive base class (protected section)
Removed the local QProcess declarations from:

LinuxDrive (was m_process)
MacDrive (was m_child)
WinDrive (was m_child)
Removed the manual m_isBusy boolean flag entirely
Implemented Drive::isBusy() to check the actual process state:
return m_process && m_process->state() != QProcess::NotRunning;
Refactored all platform implementations to use the inherited m_process

This provides a cleaner, state-based architecture as suggested. All platforms now share the same QProcess management logic, and busy status is determined by the actual process state rather than manual tracking.

Ready for review.

grulja · 2026-01-15T09:38:40Z

src/app/drivemanager.h

    RestoreStatus m_restoreStatus{CLEAN};
    QString m_error{};
    bool m_delayedWrite{false};
+    QProcess *m_process{nullptr};


Can we do here something like:

struct DriveOperationDeleter { void operator()(QProcess *process) { if (!process) { return; } if (process->state() != QProcess::NotRunning) { #ifdef Q_OS_WIN process->kill(); process->waitForFinished(); #else process->terminate(); if (!process->waitForFinished(3000)) { process->kill(); process->waitForFinished(); } #endif } delete process; } }; std::unique_ptr<QProcess, DriveOperationDeleter> m_process;

Then you can remove the m_process cleaning that is currently done separately in each [Linux,Mac,Windows]DriveManager destructor. Also you can simplify the process removal in each cancel() method and make the DriveManager::cancel() to simply do m_process.reset() that does exactly what we want thanks to the DriveOperationDeleter. With the use of std::unique_ptr you don't have to do:

if (m_process) { m_process->deleteLater(); } m_process = new QProcess(this);

but simply:

m_process = std::make_unique<QProcess>(this);

Implement mentor's suggestion to use RAII (Resource Acquisition Is Initialization) with std::unique_ptr and a custom deleter for process management. This modernizes the codebase and eliminates manual memory management issues. Changes: - Added DriveOperationDeleter struct with platform-specific cleanup logic - Changed m_process from raw QProcess* to std::unique_ptr with custom deleter - Removed all manual deleteLater() and nullptr assignments - Simplified cancel() methods across all Drive implementations - Reduced boilerplate code by ~30 lines across platforms Benefits: - Automatic cleanup prevents memory leaks even with exceptions - Centralized cleanup logic in one place (DriveOperationDeleter) - Safer and more maintainable code following modern C++ practices - Consistent behavior across Windows, Linux, and macOS platforms The DriveOperationDeleter handles: - Windows: kill() then waitForFinished() - Unix/Mac: terminate() with 3-second grace period, then kill() if needed This addresses the mentor's feedback on PR FedoraQt#914. Signed-off-by: Prachi194agrawal <[email protected]>

grulja

Almost there :)

grulja · 2026-01-15T15:58:39Z

src/app/drivemanager.cpp

+        return;
+    }
+
+    // Standard logic: Select the new drive if not busy


Nit: this comment can be removed

grulja · 2026-01-15T16:03:46Z

src/app/linuxdrivemanager.cpp

-        m_process = nullptr;
-        m_image = nullptr;
-    }
+    // Process cleanup handled automatically by unique_ptr deleter


Here we should also do m_process.reset().

grulja · 2026-01-15T16:03:59Z

src/app/linuxdrivemanager.cpp

-        m_process->deleteLater();
-        m_process = nullptr;
-    }
+    // Process cleanup handled automatically by unique_ptr deleter


Here we should also do m_process.reset().

grulja · 2026-01-15T16:04:12Z

src/app/linuxdrivemanager.cpp

    m_image->setErrorString(errorMessage);
-    m_process->deleteLater();
-    m_process = nullptr;
+    // Process cleanup handled automatically by unique_ptr deleter


Here we should also do m_process.reset().

grulja · 2026-01-15T16:05:24Z

src/app/windrivemanager.cpp

 {
-    if (m_child)
-        m_child->terminate();
+    // Process cleanup is handled automatically by std::unique_ptr with DriveOperationDeleter


I think all platform specific destructors can be removed. You can do ~WinDrive() = default in the header.

grulja · 2026-01-15T16:06:09Z

src/app/windrivemanager.cpp

-        m_child = nullptr;
-    }
+    // Reset the unique_ptr, which automatically invokes DriveOperationDeleter
+    m_process.reset();


The m_process.reset() should be moved to Drive::cancel() so we don't have to do in each platform implementation.

grulja · 2026-01-15T16:07:48Z

src/app/linuxdrivemanager.cpp

-        m_process->deleteLater();
-        m_process = nullptr;
+        // Reset the unique_ptr, which automatically invokes DriveOperationDeleter
+        m_process.reset();


Can be removed in favor of generic implementation in Drive::cancel().

- Remove redundant comment in drivemanager.cpp - Move m_process.reset() to base Drive::cancel() method - Remove duplicate reset() calls from platform-specific cancel() methods - Add explicit reset() calls in LinuxDrive completion handlers - Simplify WinDrive destructor to use default - Remove all implementation comments

grulja · 2026-01-16T07:09:50Z

src/app/linuxdrivemanager.cpp

-    if (!m_process)
-        m_process = new QProcess(this);
+    // Create new process using unique_ptr
+    m_process.reset(new QProcess(this));


→ m_process = std::make_unique<QProcess>(this);

And also remove the comment.

grulja · 2026-01-16T07:14:26Z

src/app/linuxdrivemanager.cpp

@@ -250,7 +250,7 @@
 {
    Drive::cancel();


I don't know why only the LinuxDrive does something extra in the reimplementation of cancel() but I think we can remove it altogether so none of the cancel() reimplementations exists. It wouldn't work this way anyway, because calling Drive::cancel() first it clears the m_process so the condition that is checked below will never be satisfied.

grulja · 2026-01-16T07:14:47Z

src/app/linuxdrivemanager.cpp


-    if (!m_process)
-        m_process = new QProcess(this);
+    m_process.reset(new QProcess(this));


Again, use std::make_unique

grulja · 2026-01-16T07:15:41Z

src/app/macdrivemanager.cpp

    return true;
 }

 void MacDrive::cancel()


Remove MacDrive::cancel() completely. If we don't implement it ourself the base DriveManager::cancel() will be called.

grulja · 2026-01-16T07:16:00Z

src/app/macdrivemanager.cpp

-        m_child->deleteLater();

-    m_child = new QProcess(this);
+    m_process.reset(new QProcess(this));


std::make_unique

grulja · 2026-01-16T07:16:27Z

src/app/windrivemanager.cpp

-    connect(m_child, &QProcess::readyRead, this, &WinDrive::onReadyRead);
-    connect(qApp, &QCoreApplication::aboutToQuit, m_child, &QProcess::terminate);
+    // Create new process using unique_ptr
+    m_process.reset(new QProcess(this));


std::make_unique

And drop the comment.

grulja · 2026-01-16T07:17:19Z

src/app/macdrivemanager.cpp

-    connect(m_child, &QProcess::readyRead, this, &MacDrive::onReadyRead);
-    connect(qApp, &QCoreApplication::aboutToQuit, m_child, &QProcess::terminate);
+    // Create new process using unique_ptr
+    m_process.reset(new QProcess(this));


std::make_unique and drop the comment

grulja · 2026-01-16T07:17:35Z

src/app/windrivemanager.cpp

-
-    m_child = new QProcess(this);
+
+    m_process.reset(new QProcess(this));


std::make_unique

grulja · 2026-01-16T07:23:53Z

src/app/windrivemanager.h

 public:
    WinDrive(WinDriveProvider *parent, const QString &name, uint64_t size, bool containsLive, int device, const QString &serialNumber);
-    ~WinDrive();
+    ~WinDrive() = default;


Sorry, I was wrong before, it's better not to define destructor at all in this case.

And you can also remove the destructor from LinuxDriveManager

…orm-specific cancel() overrides

Prachi194agrawal · 2026-01-16T09:10:54Z

Sir, I've updated the PR to use std::make_unique and moved the cleanup logic to the base class as suggested—thanks for the helpful guidance. Ready for another review.

grulja

Looks good to me. Thanks.

I will do some quick testing later today before I merge this to be sure it works as expected.

grulja

Sorry, you might have to revert the usage of std::make_unique as it doesn't work with custom deleter, which I didn't realize (it fails to build).

…om deleter

grulja · 2026-01-17T17:18:07Z

Can you please fix the clang-format issue? You can run clang-format locally to fix it properly. I will get to this next Friday as I will be now on vacation until then.

Prachi194agrawal · 2026-01-17T17:20:58Z

Can you please fix the clang-format issue? You can run clang-format locally to fix it properly. I will get to this next Friday as I will be now on vacation until then.

Yes, sir.

grulja requested changes Jan 13, 2026

View reviewed changes

grulja requested changes Jan 14, 2026

View reviewed changes

grulja requested changes Jan 15, 2026

View reviewed changes

Prachi194agrawal force-pushed the fix-issue-825-progress-pause branch from 7990cf9 to 9f5aa75 Compare January 15, 2026 15:23

grulja requested changes Jan 15, 2026

View reviewed changes

grulja requested changes Jan 16, 2026

View reviewed changes

Refactor QProcess management to use std::make_unique and remove platf…

f1972ee

…orm-specific cancel() overrides

grulja approved these changes Jan 16, 2026

View reviewed changes

grulja requested changes Jan 16, 2026

View reviewed changes

Fix: Revert to .reset() because make_unique is incompatible with cust…

56c55a7

…om deleter

Prachi194agrawal force-pushed the fix-issue-825-progress-pause branch from d27ef2e to 56c55a7 Compare January 16, 2026 11:58

Fix: Apply clang-format to DriveOperationDeleter

de28ba0

Prachi194agrawal requested a review from grulja January 28, 2026 14:02


		m_child = new QProcess(this);

		m_process.reset(new QProcess(this));

Fix: Prevent UI reset when new device inserted during write (#825) #914

Are you sure you want to change the base?

Fix: Prevent UI reset when new device inserted during write (#825) #914

Conversation

Prachi194agrawal commented Dec 16, 2025

Uh oh!

grulja commented Dec 22, 2025

Uh oh!

Prachi194agrawal commented Dec 25, 2025

Uh oh!

grulja commented Jan 9, 2026

Uh oh!

gastoner commented Jan 9, 2026

Uh oh!

grulja left a comment

Choose a reason for hiding this comment

Uh oh!

grulja left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prachi194agrawal commented Jan 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grulja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prachi194agrawal commented Jan 16, 2026

Uh oh!

grulja left a comment

Choose a reason for hiding this comment

Uh oh!

grulja left a comment

Choose a reason for hiding this comment

Uh oh!

grulja commented Jan 17, 2026

Uh oh!

Prachi194agrawal commented Jan 17, 2026

Uh oh!

Reviewers

Assignees

grulja left a comment •

edited

Loading