two throbbers in main window

RESOLVED FIXED in Thunderbird 12.0

Status

Thunderbird
Toolbars and Tabs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: andreasn, Assigned: mconley)

Tracking

11 Branch
Thunderbird 12.0
x86_64
All
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird11 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
It seems we have two throbbers in the main window now. One on the tabs that is currently loading and one on the main toolbar. I suggest that the one on the tab is just enough by default.
(Assignee)

Comment 1

5 years ago
Created attachment 587728 [details] [diff] [review]
Patch v1

This is my first run at it.  Removes the throbber in Windows 7 as expected.  Haven't seen what it looks like in OSX yet.
Assignee: nobody → mconley
(Reporter)

Comment 2

5 years ago
Seems to work fine on Linux. I can test on OSX tomorrow.
(Reporter)

Comment 3

5 years ago
And works fine on OSX as expected.
(Assignee)

Comment 4

5 years ago
Comment on attachment 587728 [details] [diff] [review]
Patch v1

Awesome! Well, that was pretty easy.

What do you think, Blake?
Attachment #587728 - Flags: ui-review?(bwinton)
Attachment #587728 - Flags: review?(bwinton)
Comment on attachment 587728 [details] [diff] [review]
Patch v1

Review of attachment 587728 [details] [diff] [review]:
-----------------------------------------------------------------

The code seems fine, so r=me, but...

It didn't actually remove the throbber from my existing profile, which I think we want to do, so I'm going to have to say ui-r-.
Attachment #587728 - Flags: ui-review?(bwinton)
Attachment #587728 - Flags: ui-review-
Attachment #587728 - Flags: review?(bwinton)
Attachment #587728 - Flags: review+
(Assignee)

Comment 6

5 years ago
Hm. So how do we go about disqualifying the throbber in the current toolbars, while keeping it in the customize dialog?  I wonder if this is another case where the migrateUI code I wrote would come in handy.
(Assignee)

Updated

5 years ago
Blocks: 644169
(Assignee)

Comment 7

5 years ago
Created attachment 588979 [details] [diff] [review]
Patch v2

Ok, this patch uses our fancy new migrateUI function to remove the throbber from the menubar in Linux and Windows, and from the mail-bar3 in OSX.
Attachment #587728 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Comment on attachment 588979 [details] [diff] [review]
Patch v2

Hey Sid,

Remember migrateUI?  Looks like we've got a chance to use it again.  Or is there a better way to put old default toolbar items out to pasture?

Anyhow, I hope you've got some cycles to take a peek at this.

-Mike
Attachment #588979 - Flags: review?(sagarwal)
Comment on attachment 588979 [details] [diff] [review]
Patch v2

Review of attachment 588979 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/modules/mailMigrator.js
@@ +162,5 @@
>          this._unAssert(fpbResource, collapsedResource);
>        }
>      }
>  
> +    if (currentUIVersion < 2) {

Per our IRC discussion, let's not set the pref if we error out at some point. I'd also like you to do the same for _unAssert in the v1 bit and move it to v2.
Attachment #588979 - Flags: review?(sagarwal) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 589986 [details] [diff] [review]
Patch v3

Thanks for the review, Sid!

Ok, I've moved the migration for V1 into V2 as suggested, and updated the tests.  I've changed enough, that I think it warrants a second go-over.  I hope that's alright.

-Mike
Attachment #588979 - Attachment is obsolete: true
Attachment #589986 - Flags: review?(sagarwal)
(Assignee)

Comment 11

5 years ago
Blake:

Sid has suggested that we also remove the throbber from the customize dialog, as a way of reducing our redundancy, and the number of knobs and dials that TB has.  Thoughts?

-Mike
(As per IRC, I think we should leave it in the customization dialog, at least until we get stats on whether the number of people who use it is less than the amount of problems it causes us.)

Comment 13

5 years ago
Even Firefox leaves their throbber in the customize dialog, so I think we may as well follow suit.
Comment on attachment 589986 [details] [diff] [review]
Patch v3

Review of attachment 589986 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/modules/mailMigrator.js
@@ +149,5 @@
>      this._dataSource = this._rdf.GetDataSource("rdf:local-store");
>      let dirty = false;
>  
> +    try {
> +      if (currentUIVersion < 2) {

I'd like to see a comment here explaining why we've effectively skipped v1.

@@ +185,4 @@
>        }
> +
> +      if (dirty)
> +        this._dataSource.QueryInterface(Ci.nsIRDFRemoteDataSource).Flush();

This should be in the finally block I believe.

@@ +199,1 @@
>      }

Well, technically you don't need the finally block since you're catching all errors, but I guess this is fine (as long as you move the flush call inside).
Attachment #589986 - Flags: review?(sagarwal) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 591083 [details] [diff] [review]
Patch v4 (r+'d by sid0)

Thanks for the review, Sid! I've made the corrections you asked for, and I'm doing a try build for committing.

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=cb52e9146548
Attachment #589986 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #591083 - Attachment description: Patch v4 → Patch v4 (r+'d by sid0)
(Assignee)

Comment 16

5 years ago
Created attachment 591458 [details] [diff] [review]
Patch v5 (r+'d by sid0)

I'm glad I did that try build - the tests were failing on OSX because I was using the wrong ID in test-migrate-to-rdf-ui-2.js.

That's been fixed in this patch, and the tests pass locally on OSX.
Attachment #591083 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/bc502832e7cd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
(Assignee)

Updated

5 years ago
OS: Windows 7 → All
(Assignee)

Comment 18

5 years ago
Comment on attachment 591458 [details] [diff] [review]
Patch v5 (r+'d by sid0)

I believe bwinton wanted this to co-exist with tabs-on-top.
Attachment #591458 - Flags: approval-comm-aurora?
(Assignee)

Updated

5 years ago
Depends on: 721378
Attachment #591458 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Comment 19

5 years ago
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/015e0052d398
status-thunderbird11: --- → fixed
You need to log in before you can comment on or make changes to this bug.