1006 bytes, patch
Scott MacGregor: approval184.108.40.206+
|Details | Diff | Splinter Review|
784 bytes, patch
Scott MacGregor: superreview+
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:220.127.116.11) Gecko/20070208 Mandriva/18.104.22.168-3mdv2008.0 (2008.0) Firefox/22.214.171.124 Build Identifier: Beagle is anymore able to open mail with thunderbird, because of a new little features introduced in the 2.x tree. It works well in the 1.5.x I have investigated more deeper the problem. Beagle call thunderbird with something like this : $ thundebird -mail imap://.... After some tries and some hack in this command line, I have discoverd that the call is ok and that thunderbird retrieves effectively the mail. The problem was "just" a displaying problem : it was hidden. After some battle, and a trip in the c++ code, I come with a one line fix which fixes the problem. It's pretty straightforward, so I hope you can include it and bring back beagle integration for the linux desktop users. Reproducible: Always Steps to Reproduce: 1. Double clic on an email indexed with beagle OR 2. launch directly thunderbird from CLI : thundebird -mail imap://.... Actual Results: It displays nothing Expected Results: It should display the mail passed in the uri (imap://...) I have made a small fix, please to take a look of it and tell me if there is anything wrong.
Created attachment 268943 [details] [diff] [review] Fix interoperability problem with beagle and other linux desktop tools I didn't really which file was used, so I applied the fix to both of them.
Maybe I should add that all the remote call are affected. The "news://", "mailbox://" and "abook://" is not working too.
one of these files is for Thunderbird (the mozilla/mail one), one for Seamonkey It's really unclear to me that this is the right way to fix this. This code always gets called on startup, if a folder is selected at startup, not just in the case of the command line issue (is Thunderbird not configured to select a folder on startup in your setup?) Normally, the thread pane shows fine, so I don't know why this call needs to get added here. In addition, that function doesn't pay attention to the fact that the thread pane is already loaded. In particular, I don't know if this code here has any performance impact: http://mxr.mozilla.org/seamonkey/source/mail/base/content/mailWindow.js#543
-> David : You were right in tb 1.5. It's not right anymore in the 2.x branch. The thread/mail panel is hidden and _cannot_ be displayed. That's what the function I call do : Ensure that this panel is _always_ here. This is a non sense to load an uri without seeing the panel, no ? Nevertheless, you are right to the fact that the 7 seven lines of ShowingThreadPane() can be skipped if the pane is already there. I don't really know how to check it properly and safely (I mean : be sure that it _is_ displayed), but there must be some way. The code of ShowingThreadPane() seems to me sufficiently clean that I didn't have to go further. And to answer to your question clearly : Yes, the thunderbird packaged in ubuntu, kubuntu, mandriva and even the community one suffers of this problem. You can try it for yourself : install beagle and see. It's pretty straightforward. With tb 1.5, the call is ok, with tb 2.0, it's not. And finally, let's say that it's just a misconfiguration of Thunderbird. How can a user wants to display a message (news or mail whatever, that's what the initialUri params is about) and _hide_ the panel which shows this message ? It cannot be a sane human reasonning ? No ? Do you really think that thunderbird should allow behaviour like this ? If yes, then please take time to explain me clearly, I cannot imagine a good reason. Thanks,
Michel, I'm trying to understand the problem - I'm not claiming it's a misconfiguration. It would seem to me that somehow we've skipped the code that displays the thread pane and I'd like to understand why...
Fallout from bug 329544 perhaps?
-> David : It seems Neil has found it. An other way to fix it is to modify "mail/base/content/mailWindowOverlay.xul". Something like this : - <key id="key_toggleMessagePane" keycode="VK_F8" oncommand="MsgToggleMessagePane();" disabled="true"/>+ <key id="key_toggleMessagePane" keycode="VK_F8" oncommand="MsgToggleMessagePane();"/> But I am not totally confident that it will be a good way. If this is done this way, you leave the place open for reintroducing this bug. If you force displaying panel when thunderbird is asked to display a thread, then it won't happen again.
My guess as to the right place to put a fix would be in loadStartFolder just before it calls messenger.loadURL(window, initialUri);
Adding "disabled="true" to the toggle message pane key handler fixes it? What would that have to do with anything? When this problem occurs, does anything appear on the error console?
Here is the message on the console : ================= Erreur : [Exception... "ComponentManager::CreateInstance returned failure code:" nsresult: "0x80570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE)" location: "JS frame :: chrome://global/content/bindings/browser.xml :: get_fastFind :: line 274" data: no] Fichier source : chrome://global/content/bindings/browser.xml Ligne : 274 ================= It's repeated again and again on the whole console. Something like 100 times. The last one is different : ================= Erreur : uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIMessenger.loadURL]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: loadStartFolder :: line 1001" data: no] ================= That's why I agree with neil. I think too that loadStartFolder is a good place to put the "ShowingThreadPane()" call.
Instead of calling ShowingThreadPane() in loadStartFolder, does it work to call ShowThreadPane() ? I'd rather call the method that selects the right deck, instead of calling the deck changed notification, if that makes sense...and presumably, if the right deck is already selected, calling ShowThreadPane() should be more of a noop...
I was agree with you, but it's not true. In fact, my first try was to put them both (ShowThreadPane and ShowingThreadPane). But ShowThreadPane() alone was not working, the pane didn't show at all. Maybe a fix to ShowThreadPane is necessary ?
Michel, what does your mailbox: url look like to load a message? I'm trying to reproduce this problem on Windows...
Here is one sample call generated by beagle : ./thunderbird -mail imap://firstname.lastname@example.org:143/fetch%3EUID%3E/INBOX%3E11530
Michel, when you load Thunderbird with your url successfully, running with your patch, does the folder pane come up without a selection, and the thread pane come up empty, but the message gets loaded in the message pane? If that's the case, then I've reproduced your problem&solution, although with a mailbox url instead of an imap url. And the reason the thread pane doesn't get shown as it would normally is that no folder is actually getting selected in the folder pane. Ideally, we would select the folder, and then select the message specified by the url, and this problem wouldn't happen. But at least I think I see what's going on...
That's it !
Created attachment 269674 [details] [diff] [review] does this work? If so, I'd prefer it, since it only calls ShowingThreadPane in the error case
=> David : Your patch do not work. Here is what the error console throw : Erreur : uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIMessenger.loadURL]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: loadStartFolder :: line 1001" data: no] The line 1001 is the line where I have added the call "ShowingThreadPane()".
oups ... => David : The patch in fact works, and works well. My apologies, I didn't do my test in good conditions.
Comment on attachment 269674 [details] [diff] [review] does this work? If so, I'd prefer it, since it only calls ShowingThreadPane in the error case I've landed this on the trunk, and am requesting 126.96.36.199 approval - this only affects the case where TB is launched with a URI that we can't parse into a folder and a message
Michel, are you involved with Beagle? Because I think it could be generating a different command line and actually get the correct folder and message selected...
-> David : Thanks, but not at all. I just have to make an integration between kerry, beagle and thunderbird on a kde desktop for the National Assembly in France. But I don't think that beagle devs are close-minded about changing their urls generations. They also generate urls for abook://... and news://... Currently, they have deactivated by default thunderbird support because they encounter a memory leak. They wait to find this leak before reactivating it. I had to activate it in the "configure" in order to obtain it.
Comment on attachment 268943 [details] [diff] [review] Fix interoperability problem with beagle and other linux desktop tools clearing request since we went with a variant of the patch. I'll move the approval flag to the other patch.
Comment on attachment 269674 [details] [diff] [review] does this work? If so, I'd prefer it, since it only calls ShowingThreadPane in the error case don't need approval tb 3 since I landed it already on the trunk.
marking fixed since it's on the trunk.
David, is this an item for bug 378635?
No, I don't think so - I'd love to have had it fixed by 188.8.131.52, but I didn't find out in time, unfortunately.
Can we hope to see it in the next 2.x release ? Or will it be only in 3.x branch ? Thanks,
David ask for approval. So the drivers have to decide. It's a simple patch which solves a regression since Thunderbird 1.5. David, I didn't mean that bug 378635 has to be fixed for 184.108.40.206. Instead I think this bug could be a blocker of bug 378635 due to it is a regression since 1.5.
Yes, I think it will be in the next 2.0x release, but I think 1.5 users will be offered a major upgrade to 220.127.116.11, which doesn't have the fix.
Michel, can you verify that this fix is working for you in the trunk / thunderbird 3 builds?
Scott : If you point me to where the build you speak is, I'll do it. Scott, David : Can you take a look at the issue 384160. https://bugzilla.mozilla.org/show_bug.cgi?id=384160 It's a small bug when you try to send a file from a linux desktop. It's me, under another email.
Michel, the latest trunk build you can find here: ftp://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-trunk
Michel, bug 384160 is on my todo list, thx for the reminder. I'm working on better command line support for loading messages, and I'll try to look at the other bug as soon as I can.
Henri,Scott : The nightly build "thunderbird-3.0a1pre.en-US.linux-i686.tar.bz2" is not working. The pane is shown, but there is no email in it. Sorry for the delay.
Maybe it's a side effect of the nightly build in the 3.x branch which has nothing to do with this regression. Henri: Is there nightly 2.x somewhere ? The best I have seen is "18.104.22.168-candidates/", but I don't think there will be the fix in it.
2.0.0.x nightlies are here: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-mozilla1.8/ When talking about the trunk builds, please also give the date of the build, 3.0a1pre doesn't say very much - though I assume you mean yesterday's or today's.
debugging this a little, I think it's broken for imap on the trunk (I believe I tried pop3) because we're not getting the online name out of the folder cache for some reason... The fix isn't in 2.0, though Michel verified that the patch worked for him with a 2.0 tree, as I remember. See #21
There is no pane and no mail with thunderbird-22.214.171.124pre.en-US.linux-i686.tar.gz David: you're right, no fix in 2.x branch. Sorry to announce bad news.
Created attachment 271101 [details] [diff] [review] fix imap case on trunk fix running imap url from the command line on the trunk
Michel, can you try a trunk build from tomorrow or later and see if that works for you? I landed the imap fix just now...
No fix in my today try. the versions tested was built yesterday. My next try will be on monday.
Comment on attachment 269674 [details] [diff] [review] does this work? If so, I'd prefer it, since it only calls ShowingThreadPane in the error case Past approval point for non-blockers, moving request to 126.96.36.199
The bug's still here in the current "thunderbird-188.8.131.52pre.en-US.linux-i686.tar.gz", downloaded today. I'll take a deeper look at it, it's not normal !
Michel: it's not on branch yet. You need to try a trunk (as in 3.0a1pre) build.
Magnus, you're right : here is the good news : it works for the current thunderbird-3.0a1pre.en-US.linux-i686.tar.bz2 So I change the resolution to FIXED. Regards,
Regarding to last comment => VERIFIED.
Not a 1.8 branch blocker (we wouldn't hold a release for this) but I'll leave the patch approvals for this to mscott to decide.
Comment on attachment 269674 [details] [diff] [review] does this work? If so, I'd prefer it, since it only calls ShowingThreadPane in the error case approving for 184.108.40.206
Removing checkin-needed keyword, since David's perfectly capable of checking in his own patches, and since situations like what it appears to be talking about require a backup whiteboard or comment or both saying something like "please check attachment nnn in to the 1.8.1 branch"
landed on 1.8 branch
Michel: It would be great if you could verify on the latest Thunderbird branch nightly, or the Thunderbird 220.127.116.11 candidate when it is available.