Closed Bug 88630 Opened 24 years ago Closed 24 years ago

Themes and Locales do not change when using Turbo.

Categories

(Core Graveyard :: Skinability, defect)

Product:
Core Graveyard
Component:
Skinability
Platform:
x86
Windows NT
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: kyleplynch, Assigned: ccarlen)

References

Details

(Whiteboard: [PDT+] checked in on branch and trunk)

Attachments

(3 files)

hack patch
1.58 KB, patch
Details | Diff | Splinter Review
Better patch
3.21 KB, patch
Details | Diff | Splinter Review
at the risk of drawing Tao's wrath - I'm just extending ccarlen's patch to locale switching for now
6.93 KB, patch
Details | Diff | Splinter Review
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.2) Gecko/20010628 BuildID: 2001062815 When changing themes (both Modern and Classic), I restart mozilla like the message prompts me to, but then, when I restart mozilla, It does not display the new theme. I later stopped the turbo application of mozilla, tried changing themes again, and it worked seamlessly. Reproducible: Always Steps to Reproduce: 1. Turn turbo mode on, on startup of windows 2. Start mozilla, and change a theme. 3. Restart Mozilla 4. Observe Actual Results: The theme remained exactly the same Expected Results: Changed the theme, regardless of the turbo or non-turbo mode.
confirming with win2k build 20010630.. Reporter: Workaround: You must restart your system or kill the remaining mozilla task.
Assignee: hewitt → ben
Blocks: 75599
Status: UNCONFIRMED → NEW
Component: Themes → Skinability
Ever confirmed: true
*** Bug 88840 has been marked as a duplicate of this bug. ***
Yes, you'll get the theme switch if you do 'File->Exit' and then run mozilla, since that will completely terminate mozilla. But, if you close the last window with the 'X' close control, then mozilla continues to run, and the new theme is not picked up since the browser is not reinitialized. ... and why is this assigned to nobody? This is a rather confusing situation for an end user (at minimum needs a clear release note, but a fix/hack would be better).
this really need to be owned by someone in vishy's group, also related to bug 81715
Assignee: nobody → vishy
*** Bug 89170 has been marked as a duplicate of this bug. ***
transferring keywords from dupliate bug.
Keywords: nsbeta1+, nsBranch
Whiteboard: [PDT+]
If we want to really quit when: (1) the last window is closed && (2) we're in -turbo mode && (3) a theme switch has taken place: Here's the place to add this: http://lxr.mozilla.org/mozilla/source/xpfe/appshell/src/nsAppShellService.cpp#818. We get here because of (1), we then check (2), so adding a check for (3) should do the trick.
Ben's pointed me to the way to check for a pending theme switch. I'll code this up on 07/06.
Whiteboard: [PDT+] → [PDT+] eta=07/06 on branch and trunk.
Assigning to me.
Assignee: vishy → ccarlen
Crap. Yeah, I'm also using that place, but you'll run into the same problem I did: we hit that case on turbo startup as well, since the window is opened and closed real fast, and then there aren't any more open. Ideas on resolving this?
I'm not running into that problem - the not-quit-on-last-window-closing problem anyway. The reason that it was a problem for the short-lived turbo window was that window is closed via a timer after it loads. By this point, mQuitOnLastWindowClosing has been set to TRUE. Normally, it's FALSE during startup before going into the main loop so profile mgr can open and close windows without doing wacky things. Is there some other window hitting this problem now?
Status: NEW → ASSIGNED
It apparently hasn't been set to true yet for me... I added an alert there, in the check to see if there are 0 windows left, and after the !mQuitOnLastWindowClosing. I get the alert on turbo startup, presumably because of the turbo window that's being closed... It'd be nice to be able to workaround this by temporarily setting isServerMode to false in navigator.js right before closing the window, and then back to true, but apparently the window won't close until the rest of the script is executed (and if it did, the script wouldn't execute), so it'd still be true at the point of closure. Catch 22.
But I see from http://lxr.mozilla.org/seamonkey/source/xpfe/bootstrap/nsAppRunner.cpp#980 that you're right: it's set to false, Ensure1Window is called (which is where the turbo magic happens), and then it's set to true. But since close() (which calls ::UnregisterTopLevelWindow, which is where we throw the dialog and do the theme switching stuff) happens on a timer, it could be a timing issue, no?
The problem is, in using this spot, that it might start to get crowded and really ugly if we put in special cases for exiting turbo mode. From this bug, a theme switch has to do it but there may be others: locale switch, updater, etc. I'm thinking maybe this should be done through nsIObserverService. We can't have various components reference nsIAppShellService/nsINativeAppSupport and the appshell should not know about various components which need to exit out of turbo mode. If the appshell service were an nsIObserver, varous things (like a theme switch) could just fire off a notification, and when appshell received the notification could do something general. Like get its nsINativeAppSupport, turn off turbo mode and we would then exit. A question for Ben: The theme switching pref panel sets the magic pref so that next time around the selected skin is set. What about switching the theme throught the View menu? Does it set this pref as well?
> happens on a timer, it could be a timing issue, no? Right. The problem is, the timer isn't going to be serviced until we enter the event loop at the end of main_1. Right before that, mQuitOnLastWindowClosing is set to TRUE. If the turbo magic window could be made and closed syncronously, it might make things easier. Then it would be covered by mQuitOnLastWindowClosing.
Right. But I thought we were just putting this stuff here for rtm. But if you want to do the observer stuff, I'd be happy to oblige... ;-) I still don't know why this isn't working. I took away the close-on-delay, and the close still worked for me, but I still got the alert...
Attached patch hack patch — — Splinter Review
The hack of a patch checks for the existence of the theme switch pref and, if so, allows us to exit turbo mode. Vishy, Ben, can you review?
Whiteboard: [PDT+] eta=07/06 on branch and trunk. → [PDT+] eta=07/06 on branch and trunk, need r=/sr=
The better patch uses nsIObserverService to allow things to indirectly (without direct reference inform the appshell service of events which may require an exit from -turbo mode. More cases can be added easily if need be.
Todd, I just verified on windows 98 (branch builds: 2001-07-05-06-0.9.2), using turbo, themes do not change it at all. When switching from classic to modern, there's a dialog pops up to warn the users to exit the browser and relaunch it again in order to see the themes changed. Unfortunately, when you relaunch the browser, the theme does not change.
Strange. I've been testing my patch on a branch build pulled this morning (about 6:00 PDT) and the theme switches after a restart. Time to update my tree and see if something broke in the meanwhile. BTW, I've been using the View menu to do it since, when I pulled, the prefs were not accessible.
adding ftang, tao and myself to the cc list Vishy just brought noted that we are facing the same problem with language and regional content switching. This could addressed by implementing a pref listener for general.useragent.locale and general.useragent.contentlocale. I believe i18n would be inclined to help with the implementation.
Instead of a prefs listener, why not just use the observer service like this patch allows. You would just need to call observerService->Notify() from your code, and the appshell can catch that and do the same thing it does here. Using observer service is better than pref notification because (1) it allows notification that it independent of any data. (2) the appshell already is an nsIObserver so it's cleaner than installing pref-change callbacks in addition to observer notifications.
ccarlen: sounds good, I'll try to get it to work as suggested then and attach the patches later this afternoon.
Cool. All it would take is here is: + } else if (topic.Equals(gSkinSelectedTopic) || topic.Equals("locale-switch") || topic.Equals("some-other-change")) { + if (mNativeAppSupport) + mNativeAppSupport->SetIsServerMode(PR_FALSE); You would also have to do os->AddObserver(weObserve, "locale-switch");, etc. Although slightly bulkier, I favor adding observer topics over having everything use an "exit-turbo" topic because other things may be interested in observing these topics down the road. Also, it limits mention of turbo mode concept (even though just a string constant) throughout the code base.
Two points: 1. IQA, Jon Rubin discovered that locale switching (via View menu) does not work in the branch build (2001-07-05?), while it does in the trunk build. CC him. 2. Instead of playing with individual chrome provider one by one, why not working on chromeRegistry level? My understanding is that all provider switching goes through nsChromeRegistry::SetProvider(). Therefore, send out a notification there and have registered observers respond respectively. Such approach also seems to be cleaner. just my 2cents.
Blocks: 62177
> Therefore, send out a notification there and have registered observers > respond respectively. Such approach also seems to be cleaner. That sounds really good. Worth much more than 2 cents :-)
jon, tao, the fix I worked on for locale switching (via View menu) was checked into the branch on 07/05 by ftang...
I just looked at the 06-28-06 branch build; I could not change locale using the View menu in that build either.
...and I just checked today's (07-06) branch build: locale switching via the View menu is now working.
> 2. Instead of playing with individual chrome provider one by one, why not working on chromeRegistry level? Problem with this is that, when the user switches the theme, no calls are made to chrome registry. See bug 84344. So, the skin switching notification has to be done (for now) from where it is. Ben - can you r= the patch? Blake is going to sr.
sr=blake
r=vishy
Summary: Themes do not change when using Turbo. → Themes and Locales do not change when using Turbo.
Whiteboard: [PDT+] eta=07/06 on branch and trunk, need r=/sr= → [PDT+] eta=07/06 on branch and trunk, have r,sr
Using today's build on windows 98 (branch build: 2001-07-09-05-0.9.2), switching themes if you select View > Apply Themes > Modern or Classic. There's a dialog pops up to warn the user to exit the browser and relaunch it again in order to see the themes changed. When you relaunch the browser, it changes the themes though-work fine.
pmac: have you launched the app using the -server option?
Updating and testing after Blake's check-in.
Whiteboard: [PDT+] eta=07/06 on branch and trunk, have r,sr → [PDT+] eta=07/09 on branch and trunk, have r,sr
Juraj, Here what I did. 1. go to ftp://sweetlou/products/client/seamonkey/windows/32bit/x86/ 2. Get this build: ftp://sweetlou/products/client/seamonkey/windows/32bit/x86/ 2001-07-09-05-0.9.2/ 3. Install it, make sure to click on "Enable Quick Launch (reboot required). 4. Reboot the machine. 5. Launch the browser. 6. Select View > Apply Theme.. > Modern or Classic or Edit > Preferences >Appearance > Themes > Modern or Classic 7. Switching theme works fine if you relaunch the browser.
pmac, do you have multiple profile or single profile? because in multiple profile case, Quick Launch is disabled and wont make any effect, so it probabaly would work just fine.
Conrad, can we get this in in time for the 0710 build? thanks, Vishy!
Yes. I'm planning on it. It was blocked this afternoon by some other problem. That's been resolved and I'm doing pre-checkin build & testing now on branch & trunk.
Checked into branch and trunk. Here's how you know it's working: After changing the theme, the mozilla icon in the system tray will immediately dissappear. You then know you're out of turbo mode and so, on closing the last window or doing File->Exit, the app will really exit.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+] eta=07/09 on branch and trunk, have r,sr → [PDT+] checked in on branch and trunk
It looks like this patch caused a 60 byte leak (Tinderbox says so). Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comet Dep went from Lk:2128B -> Lk:2184B on the build *during* my checkin. My files were not yet pulled until the next cycle of that build at which point the leak count was already up. Speedracer shows a 40 byte leak from my checkin that could be the string constants. In any case, not a PDT+ bug.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Cathleen, I have mutiple profiles though.
pmac, As Cathleen said, turbo mode is disabled when multiple profiles are present on the branch. So, if you have multiple profiles, you don't have turbo, and won't be affected by this. Turbo will be supported for multiple profiles when bug 86021 is fixed.
Thanks Conrad! I understand. So you mark this bug is "resolved-fix", is this one fixed in single profile? If it does, I want to test it and marks as 'verified-fixed'.
> So you mark this bug is "resolved-fix", is this one fixed in single profile? It's not so much that it's fixed in the single profile case as that it's not an issue in the multiple profile case. As far as verifying it, see my comment from 2001-07-10 01:10
Thanks for your quick respond, Conrad!
Verified on windows 98 (branch build: 2001-07-10-05-0.9.2) with single profile, it works. It let you switch themes when you relaunch the browser.
Status: RESOLVED → VERIFIED
See bug 89531 and bug 90279; a result of the patch for this bug?
If so, jbetak should look at it. He contributed the portions of this patch having to do with locale.
ccarlen: I agree and do feel responsible for the contributed functionality in this patch. jon: we need to coordinate with tao for this verification, since his checkin for bug 86807 on 07/09 changed the way UI and region locale switching works. Starting with builds dated 07/10, language packs with the appropriate version number are required, otherwise the chrome registry will not perform the switch. Seems like we definitely need some up-to-date or at least dummy packs soon...
No longer blocks: 75599
Blocks: 75599
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: