Closed Bug 715565 Opened 8 years ago Closed 8 years ago

two throbbers in main window

Categories

(Thunderbird :: Toolbars and Tabs, defect)

11 Branch
x86_64
All
defect
Not set

Tracking

(thunderbird11 fixed)

RESOLVED FIXED
Thunderbird 12.0
Tracking Status
thunderbird11 --- fixed

People

(Reporter: andreasn, Assigned: mconley)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
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
Seems to work fine on Linux. I can test on OSX tomorrow.
And works fine on OSX as expected.
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+
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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
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+
Attached patch Patch v3 (obsolete) — Splinter Review
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)
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.)
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+
Attached patch Patch v4 (r+'d by sid0) (obsolete) — Splinter Review
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
Attachment #591083 - Attachment description: Patch v4 → Patch v4 (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
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/bc502832e7cd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
OS: Windows 7 → All
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?
Attachment #591458 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.