Skip to content

feat: show busy dialog while parsing archive contents for extract#2468

Open
Fikri-20 wants to merge 4 commits intoborgbase:masterfrom
Fikri-20:fix/extract-progress-dialog
Open

feat: show busy dialog while parsing archive contents for extract#2468
Fikri-20 wants to merge 4 commits intoborgbase:masterfrom
Fikri-20:fix/extract-progress-dialog

Conversation

@Fikri-20
Copy link
Copy Markdown

Fixes #1570

When extracting from a large archive, parsing the file list can take minutes but the UI only showed a small status-bar message. Added a QProgressDialog in indeterminate mode that appears when parsing starts and closes when done.

_set_status() in ChangeBorgPassphraseWindow called self.errorText.setText()
but the widget was absent from the UI file, causing an AttributeError crash
when displaying errors after a keyring change.

Adds the errorText QLabel (matching the pattern used in repo_add.ui) and a
regression test that verifies _set_status() works without crashing.

Fixes borgbase#2454
@Fikri-20
Copy link
Copy Markdown
Author

hey @m3nu , could you please review the PR and tell me if I need to tweak anything else?

@Fikri-20 Fikri-20 force-pushed the fix/extract-progress-dialog branch from 87652bf to 7637837 Compare March 25, 2026 22:12
@Fikri-20
Copy link
Copy Markdown
Author

Hi @m3nu, I'm applying for GSoC 2026 and have these PR ready for review. Could you please take a look?

Copy link
Copy Markdown
Contributor

@m3nu m3nu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good feature for #1570 -- the indeterminate QProgressDialog is the right UX pattern here. A few items to address before merge:

Should fix:

  1. Clean up _extract_progress reference after close. The QProgressDialog remains referenced on self.tab._extract_progress indefinitely after closing. Either set it to None or use deleteLater():

    self.tab._t.finished.connect(self.tab._extract_progress.close)
    self.tab._t.finished.connect(self.tab._extract_progress.deleteLater)
  2. Remove or unify redundant _set_status() call. The status bar message "Processing archive contents" is now redundant since the modal dialog shows the same information. Either remove the _set_status call or keep it as a fallback and remove the near-duplicate translatable string.

Nice to have:

  1. Strengthen the test. The current test only checks hasattr(tab, '_extract_progress') after completion, which is trivially true once the attribute is set. A stronger test would verify the dialog was actually visible at some point (e.g., mocker.spy on QProgressDialog.show) or that close was called.

  2. Ellipsis consistency. The dialog string uses Unicode ellipsis () while the status bar version doesn't. Minor, but these are two separate translation keys -- consider unifying.

@Fikri-20 Fikri-20 force-pushed the fix/extract-progress-dialog branch from 7637837 to 31f276f Compare March 29, 2026 01:51
@Fikri-20
Copy link
Copy Markdown
Author

Fikri-20 commented Mar 29, 2026

Hey @m3nu, thanks for the thorough review. working on them!

@Fikri-20 Fikri-20 force-pushed the fix/extract-progress-dialog branch from 31f276f to 53cd525 Compare March 29, 2026 02:02
@Fikri-20
Copy link
Copy Markdown
Author

Just pushed the fixes for all your comments:
✓ deleteLater() to clean up the progress dialog
✓ Removed the redundant _set_status() call
✓ Strengthened the test with mocker.spy
✓ Also removed an unrelated file that slipped in
It should be good for another look now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show extract status more prominently.

2 participants