813725 - Top of RSS item viewing window frozen

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[epaul137ml]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121119183901

Steps to reproduce:

I opened a RSS entry viewing window by double clicking on a RSS item. This displayed the RSS item in a new window.


Actual results:

After viewing and deleting the RSS item, the bottom of the window updated to display the next RSS item but the top of the window is still showing the title and http link to the item that was deleted. After viewing and deleting multiple items, the top of window still displays the information from the first item that was displayed in the window. Closing the window and doubling clicking the next RSS item, starts the issue over.


Expected results:

The top of the window should follow the bottom of the window. The window operated correctly in Thunderbird 16 and below.

Comment 1

[epaul137ml]

The problem only occurs if you reading RSS items from the top down. If you are reading items from the bottom up, the top of the window updates properly.

Comment 2

[alta88]

hmm, this happens on message deletion in either the standalone window or 3pane (regardless if the message opened initially is first or last in threadpane) only if Message->Open Feed message is Web Page.  the standalone window headers never update even though the right web page loads (for next message after deletion).

Comment 3

[alta88]

Attachment: patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 741662 [details] [diff] [review]
patch

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

::: mail/base/content/folderDisplay.js
@@ +1262,5 @@
>      let viewIndex = this.view.dbView.currentlyDisplayedMessage;
>      let msgHdr = (viewIndex != nsMsgViewIndex_None) ?
>                     this.view.dbView.getMsgHdrAt(viewIndex) : null;
>  
> +    if ((this instanceof FolderDisplayWidget) &&

I don't think it's acceptable to do instanceof here. 
Override in StandaloneFolderDisplayWidget as needed instead?

Comment 5

[alta88]

why exactly?  StandaloneFolderDisplayWidget calls FolderDisplayWidget proto.  having to recreate the displayMessageChanged() function in SFDW without these 2 lines seems quite inefficient.

Comment 6

[Magnus Melin [:mkmelin]]

StandaloneFolderDisplayWidget is supposed to handle standalone window differences from FolderDisplayWidget. If we start testing for standalone inside FolderDisplayWidget it's a slippery slope... 

You don't necessarily need to copy the implementation. You could create a helper method in FolderDisplayWidget that handles setting the content, then override that metod in StandaloneFolderDisplayWidget to do nothing. 

Or maybe there's better ways - i'm not sure i understand what the problem was?

Comment 7

[alta88]

Attachment: patch

well, i guess i still don't understand why it's (too) slippery.  yes, there are numerous other methods, but there are 2 subclasses of FDW in the codebase, and i wanted to do it in < 1 line.. ;)

fortunately it turns out the subclasses already call the super with a null aTabInfo (for the same reasons) and this will work perfectly (better).

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 742368 [details] [diff] [review]
patch

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

Seems to be some problem with the diff

$ hg qpush -v
applying feedWindow.patch
patching file mail/base/content/folderDisplay.js
bad hunk #1 @@ -1258,17 +1258,20 @@ FolderDisplayWidget.prototype = {
 (18 17 20 20)
patch failed, rejects left in working dir
errors during apply, please fix and refresh feedWindow.patch

Comment 9

[alta88]

Attachment: updated

updated..

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 742505 [details] [diff] [review]
updated

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

::: mail/base/content/folderDisplay.js
@@ +1269,1 @@
>        FeedMessageHandler.setContent(msgHdr, false);

I think this if should have braces since thers's the comment in there.

also ie -> i.e. and "a tabInfo"

Comment 11

[alta88]

Attachment: for checkin

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/13b289848f1d

Comment 13

[marvinhk]

will this be deployed to earlybird or beta soon?
or this will be known issue until tb23?
I'd like to check out Bug 816414 which is said to have this as root cause.
not sure if I should wait for tb22 beta upon code merge or nightly directly.