1565007 - When saving multiple attachments, save directory/folder not remembered between saves: Save-as dialogs open simultaneously instead of consecutively

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Kamens]

I observed this behavior on Linux under GNOME:

1. View a message containing multiple attachments.
2. Expand the attachment bar so that the list of individual attachments is visible.
3. Select two of the attachments, right-click on them, and select "Save as...".
4. In the same pop-up, enter the full path of the location to save the first attachment, making sure that it is in a different directory from where the save pop-up defaulted to when it first opened up.
5. Note how when the pop-up for saving the second attachment comes up, it opens in the same directory that the first pop-up did, i.e., it doesn't remember the new directory into which you saved the first attachment.

This is a regression. I ran mozregression in it:

11:03.27 INFO: Running comm-central build built on 2019-04-03 21:58:16.259000, revision a240e0e2
11:14.76 INFO: Launching /tmp/tmppX5WJD/thunderbird/thunderbird
11:14.76 INFO: Application command: /tmp/tmppX5WJD/thunderbird/thunderbird --allow-downgrade -profile /tmp/tmpLIZj5K
11:14.76 INFO: application_buildid: 20190403211531
11:14.76 INFO: application_changeset: a240e0e27ce5c54a13857b4046f797eff255a4c4
11:14.76 INFO: application_name: Thunderbird
11:14.76 INFO: application_repository: https://hg.mozilla.org/comm-central
11:14.76 INFO: application_version: 68.0a1
Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): bad
11:32.83 INFO: Narrowed inbound regression window from [46eb5a67, ea892780] (3 builds) to [46eb5a67, a240e0e2] (2 builds) (~1 steps left)
11:32.83 INFO: No more inbound revisions, bisection finished.
11:32.83 INFO: Last good revision: 46eb5a67c0893571784c5e8c8462a58b552fe893
11:32.83 INFO: First bad revision: a240e0e27ce5c54a13857b4046f797eff255a4c4
11:32.83 INFO: Pushlog:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=46eb5a67c0893571784c5e8c8462a58b552fe893&tochange=a240e0e27ce5c54a13857b4046f797eff255a4c4

Comment 1

[Jorg K (CEST = GMT+2)]

Still working OK on Windows. Maybe related to bug 1345167 although I doubt it.

Some "content pref" stores the last directory, well, maybe on a URL basis, where in this case the URL would be the URL of the message attachment.

Comment 2

[alta88]

It seems all folderpicker save windows are opened at once now on linux for multiple attachments, instead of resolving one (which would save the messenger.save.dir pref) and then opening another, It could be a matter of an await somewhere. But it doesn't make sense if that's not the behavior on win.

Comment 3

[Jonathan Kamens]

Is anyone looking at this regression?

Comment 4

[Jorg K (CEST = GMT+2)]

Doesn't look like it.

Comment 5

[Jonathan Kamens]

Let me rephrase the question: is there anything I can do to cause someone to fix this?

I don't think we want to ship TB78 with this bug in it, so somebody's going to have to look at it eventually, and it seems like sooner would be preferable to later.

Comment 6

[Wayne Mery (:wsmwk)]

And also not bug 1526811 ?

Comment 7

[Jorg K (CEST = GMT+2)]

OK, I was all wrong in comment #1. After saving the first attachment into a new location, it loses that location for the second. Confirmed.

Alta88, could you please take another look.

Comment 9

[Thomas D. (:thomas8)]

Adjusting OS per comment 7 (and also confirmed by myself); this is also seen on Windows.

I believe that Alta88's analysis of comment 2 is mostly correct: Just opening one dialog per attachment sequentially as we used to do (instead of opening all of them at once, which doesn't scale and behave well) will solve this annoying bug.

Comment 10

[Thomas D. (:thomas8)]

...

Comment 11

[Thomas D. (:thomas8)]

Confirming (should have been confirmed before). Pretty annoying; will surely see more dupes of this.

Please note:

• The regression is NOT that we are opening one dialog for each attachment (which is correct and accepted behaviour of "Save As" command since time immemorial); the regression is that we are now opening all dialogs at once (i.e. all on the same folder used for the last save) instead of one after the other (where user may freely choose folder AND file name for each file, or just press ENTER on each file if he's happy with folder and file name, and we'll remember the last used folder each time for the next attachment).
• Until this bug, users have never complained about having multiple (consecutive) dialogs because it was easy enough to just ENTER them away for saving into one target folder, whilst having full flexibility to chose different folder and/or file name for each attachment if/as required.
• Different users, different needs: I see no reason to remove this feature, we should start by just fixing the regression.
• Saving multiple attachments into one folder with only one folder-select dialog (i.e. without possibility of adjusting folder / file name for each attachment) is an RFE filed as bug 1595732, which does not prejudice fixing this regression.

Comment 12

[Wayne Mery (:wsmwk)]

NI Magnus for prioritization of this regression

Comment 16

[Thomas D. (:thomas8)]

Wayne, does the regressionwindow at the end of comment 0 help?

Comment 17

[Magnus Melin [:mkmelin]]

This should be from making the pickers open async.

Comment 19

[jerzy.orlowski]

Hi,
I submitted a duplicate of this bug and I can say that it is very annoing. I frequently find that multiple attachments I want to save are not in the directory I thin they are.
To better illustrate how important that is, I spent more than one hour figuring out how github and bugzilla works even though I am not a developer.

I even consider changing Thunderbird o another system because oft he bug.

Comment 20

[Michael Weghorn]

Attachment: Bug 1565007 - For Save As of multiple attachments, open save dialogs sequentially to remember previous folder path. r?thomasd,mkmelin

Presumably since changeset 26228:d167921e42eb
("Bug 1345167 - Enhance UI display/feedback for external (deleted, detached to
file, and http link) attachments.", 2019-03-31), saving multiple attachments
is done asynchronously, and the multiple dialogs were spawned in parrallel.

Initializing multiple dialogs at once was irritating and prevented preselecting
the directory into which the previous attachment was saved in the dialog of the
next attachment to be saved (s. bug 1565007 / bug 1607695).

Now we will only show one dialog at a time for Save As of multiple attachments.
The save path from each previous attachment is now properly remembered
for the next one again.

Comment 22

[Thomas D. (:thomas8)]

In duplicate bug 1607695, new contributor Michael W. had provided a patch some months ago, which imo fixes this bug quite elegantly.
Magnus said r- citing focus problems. Assignee had questions on the review which unfortunately hadn't received a reply until now.

I have rebased and tested the patch locally (not yet attached here), and it still works perfectly, exactly as expected. For certain click patterns involving the Save button on attachment header, indeed focus is force-changed to the message list a bit prematurely as the slower file picker opens async and is overtaken by the subsequent focus change. However, that minor disorder has been there before the patch, and it doesn't seem to cause any problems as save dialogs are always correctly focused, so we can fix that in a follow-up bug.

I suggest to rebase this and land essentially as-is, with some nits like var vs. let and comment capitalization.

Comment 23

[Thomas D. (:thomas8)]

(In reply to Michael Weghorn from Bug 1607695 Comment 12)

(In reply to Thomas D. (:thomas8) from Bug 1607695 comment #10)
Hi Thomas, thanks for your comment and "reviving" this, as well as the tip how to properly handle such situations in the future!
[...] Thanks for the explanation. That's the part I didn't really understand from the review myself.

Kindly let us know if you are still willing and able to continue with this patch - we appreciate your contribution! Again, sorry for the undue delay.

Yes, I'd be happy to continue, it might just take me a while to get my development setup up and running again.

Comment 24

[Michael Weghorn]

(In reply to Thomas D. (:thomas8) from comment #22)

[...]
I suggest to rebase this and land essentially as-is, with some nits like var vs. let and comment capitalization.

Should I already update the patch accordingly or first wait for more opinions on the suggestion to go with this approach in general?

Comment 25

[Thomas D. (:thomas8)]

Imo, it would be good to have a rebased & updated version of your patch for testing purposes. Thank you!

Comment 26

[Michael Weghorn]

I have now submitted a rebased and updated version of the patch ( https://phabricator.services.mozilla.com/D76327 ) that tries to address the suggestions (uses 'let' instead of 'var', capitalized comments). I'd be happy to hear if any other changes should be made.

Comment 27

[Thomas D. (:thomas8)]

(In reply to Michael Weghorn from comment #26)

I have now submitted a rebased and updated version of the patch ( https://phabricator.services.mozilla.com/D76327 ) that tries to address the suggestions (uses 'let' instead of 'var', capitalized comments). I'd be happy to hear if any other changes should be made.

Thank you very much, Michael! This seems to fix the regression without any adverse effects on the current focus patterns, which remain the same before and after the patch. Let's hear from Magnus if we can roll with this.

Magnus, could you please have another look in view of my comment 22?

(In reply to Thomas D. (:thomas8) from comment #22)

In duplicate bug 1607695, new contributor Michael W. had provided a patch some months ago, which imo fixes this bug quite elegantly.
Magnus said r- citing focus problems. Assignee had questions on the review which unfortunately hadn't received a reply until now.

I have rebased and tested the patch locally [now attached here by assignee], and it still works perfectly, exactly as expected. For certain click patterns involving the Save button on attachment header, indeed focus is force-changed to the message list a bit prematurely as the slower file picker opens async and is overtaken by the subsequent focus change. However, that minor disorder has been there before the patch, and it doesn't seem to cause any problems as save dialogs are always correctly focused, so we can still polish that in a follow-up bug if we really want.

I suggest to rebase this [done] and land essentially as-is, with some nits like var vs. let and comment capitalization [done].

Comment 28

[Michael Weghorn]

Hi Magnus, could you possibly take another look at https://phabricator.services.mozilla.com/D76327, now that Thomas's comments have been addressed and he is fine with it?

In D76327#3214158, @ThomasD wrote:

LGTM, thanks Michael! As I mentioned before, this patch does not change focus over the status quo, so it would be great to see this workflow breaker fixed asap.

Over to Magnus.

Comment 29

[Magnus Melin [:mkmelin]]

Yes it's on my list.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/02cdb182c495
For Save As of multiple attachments, open save dialogs sequentially to remember previous folder path. r=thomasd,mkmelin

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 9188212 [details]
Bug 1565007 - For Save As of multiple attachments, open save dialogs sequentially to remember previous folder path. r?thomasd,mkmelin

[Approval Request Comment]
User impact if declined: improves user experience when saving multiple attachments
Testing completed (on c-c, etc.): on beta
Risk to taking this patch (and alternatives if risky): not risky

Comment 32

[Wayne Mery (:wsmwk)]

Comment on attachment 9188212 [details]
Bug 1565007 - For Save As of multiple attachments, open save dialogs sequentially to remember previous folder path. r?thomasd,mkmelin

[Triage Comment]
Approved for esr78

Comment 33

[Rob Lemley [:rjl]]

Thunderbird 78.8.1:
https://hg.mozilla.org/releases/comm-esr78/rev/3bf175d022ae

Comment 34

[Michael Weghorn]

Thomas D. asked me to do a quick retest with 78.8.1. I can confirm the behaviour is as intended there again (tested on Debian with Thunderbird 78.8.1 downloaded from Thunderbird website).