Last modified: 2012-09-25 19:00:02 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T14701, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 12701 - "(last change)" in new messages box should link to combined diff of all changes since last visit
"(last change)" in new messages box should link to combined diff of all chang...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Interface (Other open bugs)
unspecified
All All
: Normal enhancement with 10 votes (vote)
: ---
Assigned To: Niklas Laxström
: patch, patch-reviewed, schema-change
: 8216 16314 (view as bug list)
Depends on:
Blocks: messages 16012
  Show dependency treegraph
 
Reported: 2008-01-20 11:43 UTC by Roan Kattouw
Modified: 2012-09-25 19:00 UTC (History)
23 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
proposed patch (7.71 KB, patch)
2009-05-18 14:10 UTC, Ahmad Sherif
Details

Description Roan Kattouw 2008-01-20 11:43:40 UTC
When your user talk page is changed, a box shows up saying "You have new messages (last change)" where "(last change)" links to the diff of the last revision of your user talk page. This should be changed to "(last changes)", linking to a combined diff of all changes you haven't seen yet (URL: index.php?diff=cur&oldid=NNNN). This would require a schema change, adding a revid field to the user_newtalk table. That field would record the revid of the last revision the user has seen.

To clarify:

- Someone edits my talk page, creating revision 12345
- A row is added to the user_newtalk table, with 12333 (the previous revision) as revid
- More edits are made; the user_newtalk table isn't touched
- I view a page and see a banner linking to diff=cur&oldid=12333
- I view my user talk page, causing the row in the user_newtalk page to be deleted
Comment 1 Bogdan Stancescu 2008-01-20 15:32:22 UTC
How does the current version of the code know when you last visited your talk page? There has to be a timestamp somewhere storing that information. Can't that timestamp be cross-checked with the talk page's revision history, thus determining the previous revision number? That would be your NNNN, and you wouldn't need any schema changes.
Comment 2 Roan Kattouw 2008-01-20 15:40:22 UTC
(In reply to comment #1)
> How does the current version of the code know when you last visited your talk
> page? There has to be a timestamp somewhere storing that information.
There isn't. Whenever someone edits my talk page, a row is added to the user_newtalk table (see http://www.mediawiki.org/wiki/User_newtalk_table ), triggering the new messages box. When I view my talk page, that row is deleted again.
Comment 3 Platonides 2008-01-20 17:45:59 UTC
I think this is already done by enotif?
http://www.mediawiki.org/wiki/Enotif suggest it's completely done by enotif, but if it wasn't the Update markers would simplify much of the work.
Comment 4 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-01-20 19:06:04 UTC
A timestamp would probably be better to store than a rev_id.  It contains more information than rev_id, and also avoids some conceivable difficulties with referential integrity (e.g., revision is oversighted, now how do you get the diff?).
Comment 5 Bogdan Stancescu 2008-01-20 20:02:43 UTC
(In reply to comment #4)
> A timestamp would probably be better to store than a rev_id.

True, but storing the rev_id is slightly less expensive, because you don't have to perform the extra step of actually identifying the rev_id when the time comes to show the diff (you already have it).

Also, a timestamp indicating what event?
Comment 6 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-01-20 20:18:41 UTC
(In reply to comment #5)
> True, but storing the rev_id is slightly less expensive, because you don't have
> to perform the extra step of actually identifying the rev_id when the time
> comes to show the diff (you already have it).

One extra query that involves B-tree lookup of a single index entry is trivial.

> Also, a timestamp indicating what event?

The last time the viewer looked at the most recent revision of the page.
Comment 7 Bogdan Stancescu 2008-01-20 20:43:44 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Also, a timestamp indicating what event?
> 
> The last time the viewer looked at the most recent revision of the page.
> 

Then, if comment #2 is correct (which I assume it is), you're basically proposing a rewrite of this feature (currently there's no entry in table user_newtalk at the moment when a viewer looks at the most recent revision of the page -- on the contrary, that's when the entry is deleted).
Comment 8 Huji 2008-01-20 20:49:11 UTC
Maybe we can use a rev_id like this: If the rev_id is there, voila! Otherwise, find the biggest rev_id which is smaller than the expecte one (i.e. the last remaining revision before the one stored in user_newtalk). For certain, this algorithm should be implemented in the diff code... which sounds... irksome! Sorry for my flight of idea.
Comment 9 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-01-20 21:05:21 UTC
(In reply to comment #7)
> Then, if comment #2 is correct (which I assume it is), you're basically
> proposing a rewrite of this feature (currently there's no entry in table
> user_newtalk at the moment when a viewer looks at the most recent revision of
> the page -- on the contrary, that's when the entry is deleted).

Well, yes, since you point it out.  More conveniently, we could make the timestamp be the timestamp of the insert.  This would be almost completely equivalent, since the interval between the last view and the first unviewed change is irrelevant to this feature.  The only catch would be if two pages were merged, or revisions were otherwise inserted out-of-sequence.  In that case the current schema fails however you look at it, unless the merge alters the row (which it could be arranged to do, if desired).

Either way, it's cleaner to use a timestamp than a rev_id, since the important information is not really dependent on the specific rev_id, and so using a rev_id could *conceivably* cause referential-integrity problems as a consequence.  Not that I view it as likely, but we may as well do The Right Thing to start with, if it costs nothing more.

(In reply to comment #8)
> Maybe we can use a rev_id like this: If the rev_id is there, voila! Otherwise,
> find the biggest rev_id which is smaller than the expecte one (i.e. the last
> remaining revision before the one stored in user_newtalk).

Also possible, if you assume rev_id is strictly increasing (I'm not sure it is, but it's certainly fairly close).  But it's easier to just use a timestamp.
Comment 10 Bogdan Stancescu 2008-01-20 21:27:10 UTC
(In reply to comment #9)
> Also possible, if you assume rev_id is strictly increasing (I'm not sure it is,
> but it's certainly fairly close).  But it's easier to just use a timestamp.

From all I've seen, rev_id /is/ strictly increasing (I didn't look into the code, but I'm making this assumption in related code, and I never saw a failure).

I was thinking about the same solution as the one suggested by Huji in comment #8, but with a twist: instead of implementing that "smartness" in the specific "you have new messages" code, implement it in the algorithm that shows the actual diff. That way you solve all referential integrity problems with diffs.

Simetrical, your proposal does make sense, but I think it incurs unnecessary penalties for both coding (redesign as opposed to a minor hack) and execution times (if you think it through, you'll probably find that you need to make timestamp comparisons between two tables on every page being served, as opposed to checking whether a field exists in a single table based on a primary key). But then again, that's just my 2c.
Comment 11 Huji 2008-01-21 08:00:21 UTC
Yes. The rev_id is strictly increasing because it is created like this (pasting SQL code):

CREATE TABLE /*$wgDBprefix*/revision (
  rev_id int unsigned NOT NULL auto_increment,
Comment 12 Roan Kattouw 2008-01-21 14:50:57 UTC
(In reply to comment #11)
> Yes. The rev_id is strictly increasing because it is created like this (pasting
> SQL code):
> 
> CREATE TABLE /*$wgDBprefix*/revision (
>   rev_id int unsigned NOT NULL auto_increment,
> 

That doesn't necessarily hold when two histories are merged: in that case, a rev with a lower rev_id could be more recent.

I think the timestamp is probably the cleanest way to go, even though it requires an extra query (which is indexed anyway).
Comment 13 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-01-21 22:59:38 UTC
(In reply to comment #11)
> Yes. The rev_id is strictly increasing because it is created like this (pasting
> SQL code):
> 
> CREATE TABLE /*$wgDBprefix*/revision (
>   rev_id int unsigned NOT NULL auto_increment,

Depending on the configuration, under high insert load autoincrements might not be increasing in time due to race conditions, at least conceivably.  Probably MySQL-side race conditions are rare enough to be negligible for minor UI details like this, but I'm not sure offhand that there are no application-side race conditions.  If the timestamp is calculated near the beginning of the request, say, then processing before the insert could easily cause reordering in some cases.  I tried checking for such possibilities by querying a simplewiki dump on localhost, but the only query I could think of was a Cartesian join on the revision table, which promptly ate up all my RAM, started swapping, and froze my computer (had to use Alt-SysRq-K), so my empirical attempt to test this failed.  :)

Anyway, probably anything has corner cases (using timestamps runs into issues with two revisions of the same page in the same second).  It's really not terribly important if there's a rare UI oddity, and if there is we can always fix it later.  There's no point in talking about it, someone should just do it, using whatever method they feel like.
Comment 14 Aaron Schulz 2008-09-28 16:09:11 UTC
Already working on this
Comment 15 Aaron Schulz 2008-11-11 21:21:44 UTC
*** Bug 16314 has been marked as a duplicate of this bug. ***
Comment 16 Melancholie 2009-03-27 01:56:52 UTC
*** Bug 8216 has been marked as a duplicate of this bug. ***
Comment 17 Melancholie 2009-03-27 02:07:23 UTC
When watching a page, it gets changed and you visit its history before viewing itself, you get that cool green 'updated' marker. This means that an oldid info is somewhere in the recentchanges table or probably watchlist table (also used by ENotif!).

It would be nice to have that available for the notification box too, as with SUL you just leave a message somewhere, not knowing if the user will actually see/read it (some users get literally "spammed" with talk messages etc.).
Comment 18 Ahmad Sherif 2009-05-18 14:10:05 UTC
Created attachment 6129 [details]
proposed patch

Maybe this patch would help. It also adds how many unreaded messages you have in the notification.
Comment 19 Roan Kattouw 2009-05-18 14:37:54 UTC
(In reply to comment #18)
> Created an attachment (id=6129) [details]
> proposed patch
> 
> Maybe this patch would help. It also adds how many unreaded messages you have
> in the notification.
> 

Adding Title::getPreviousRevisionIDAtOffset() is of no use, since we've got Revision::getPrevious() already. Also, there's only one entry per user in the user_newtalk table AFAIK, so returning the number of entries is useless as well in that case.

Also, instead of using the 'newmessage' and 'newmessages' messages, you should use {{PLURAL:}}.
Comment 20 Ahmad Sherif 2009-05-18 16:22:51 UTC
(In reply to comment #19)
> Adding Title::getPreviousRevisionIDAtOffset() is of no use, since we've got
> Revision::getPrevious() already. Also, there's only one entry per user in the
> user_newtalk table AFAIK, so returning the number of entries is useless as well
> in that case.
> 
There is an entry for every edit for user's talk page. At least that's what in user_newtalk in my local db. So, there will be one entry or more, so i don't think that Revision::getPrevious() would help me that much.

> Also, instead of using the 'newmessage' and 'newmessages' messages, you should
> use {{PLURAL:}}.
> 
This could be done, but after reviewing the patch to see if it can be committed to the trunk or thrown in the trash :).
Comment 21 Roan Kattouw 2009-05-18 16:35:56 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > Adding Title::getPreviousRevisionIDAtOffset() is of no use, since we've got
> > Revision::getPrevious() already. Also, there's only one entry per user in the
> > user_newtalk table AFAIK, so returning the number of entries is useless as well
> > in that case.
> > 
> There is an entry for every edit for user's talk page. At least that's what in
> user_newtalk in my local db. So, there will be one entry or more,
Right.

> so i don't
> think that Revision::getPrevious() would help me that much.
> 
Those two aren't related. My point is that you're duplicating code from Revision::getPrevious() to Title::getPreviousRevisionIDAtOffset() for no good reason. To get the previous revision ID for a certain revision ID you should really just create a Revision object for it (if you don't have one already) with Revision::newFromID() and call getPrevious() on it.

> > Also, instead of using the 'newmessage' and 'newmessages' messages, you should
> > use {{PLURAL:}}.
> > 
> This could be done, but after reviewing the patch to see if it can be committed
> to the trunk or thrown in the trash :).
> 
... or you could do it now; developers are typically more hesitant to commit patches that need fixing up.
Comment 22 Ahmad Sherif 2009-05-18 17:02:25 UTC
(In reply to comment #21)
> Those two aren't related. My point is that you're duplicating code from
> Revision::getPrevious() to Title::getPreviousRevisionIDAtOffset() for no good
> reason. To get the previous revision ID for a certain revision ID you should
> really just create a Revision object for it (if you don't have one already)
> with Revision::newFromID() and call getPrevious() on it.
> 

AFAIU, Revision::getPrevious(), just like Title::getPreviousRevisionID, gives me *only* the previous revision, I need, for example, 5th revision before the current revision, so I will have to use a loop or something using Revision::getPrevious() to get only the rev id to put it in the link. Title::getPreviousRevisionIDAtOffset() gives me the rev id directly or I'm missing something.

> ... or you could do it now; developers are typically more hesitant to commit
> patches that need fixing up.
> 

On a second thought, will {{PLURAL:...}} allow us to make a link from 'a new message' or 'new messages'? Sorry if the question is stupid :).
Comment 23 Roan Kattouw 2009-05-18 17:26:24 UTC
(In reply to comment #22)
> (In reply to comment #21)
> > Those two aren't related. My point is that you're duplicating code from
> > Revision::getPrevious() to Title::getPreviousRevisionIDAtOffset() for no good
> > reason. To get the previous revision ID for a certain revision ID you should
> > really just create a Revision object for it (if you don't have one already)
> > with Revision::newFromID() and call getPrevious() on it.
> > 
> 
> AFAIU, Revision::getPrevious(), just like Title::getPreviousRevisionID, gives
> me *only* the previous revision, I need, for example, 5th revision before the
> current revision, so I will have to use a loop or something using
> Revision::getPrevious() to get only the rev id to put it in the link.
> Title::getPreviousRevisionIDAtOffset() gives me the rev id directly or I'm
> missing something.
> 
Hmm. This is probably justified, but it still gives me the creeps. Implementing this more cleanly would require a schema change though.

> > ... or you could do it now; developers are typically more hesitant to commit
> > patches that need fixing up.
> > 
> 
> On a second thought, will {{PLURAL:...}} allow us to make a link from 'a new
> message' or 'new messages'? Sorry if the question is stupid :).
> 
It'll allow something like {{PLURAL:$1|a new message|$1 new messages}} where $1 is substituted with the number of messages.
Comment 24 Ahmad Sherif 2009-05-18 17:31:40 UTC
(In reply to comment #23)
> It'll allow something like {{PLURAL:$1|a new message|$1 new messages}} where $1
> is substituted with the number of messages.
> 

Yes I know, but I'm talking about 'a new message' or '$1 new messages' being a link to the talk page.
Comment 25 Roan Kattouw 2009-05-18 17:53:09 UTC
(In reply to comment #24)
> Yes I know, but I'm talking about 'a new message' or '$1 new messages' being a
> link to the talk page.
> 
That should still work; those two are independent.
Comment 26 DieBuche 2011-04-14 10:49:31 UTC
Patch needs to be updated, doesn't apply cleanly anymore
Comment 27 Sumana Harihareswara 2012-05-25 03:03:09 UTC
Ahmad Sherif: Thanks again for the patch.  I've marked it "reviewed" because it no longer cleanly applies to our codebase.  Are you interested in updating it, and then in using developer access to directly suggest it into our Git source control system?

https://www.mediawiki.org/wiki/Developer_access

https://www.mediawiki.org/wiki/Git/Workflow#How_to_submit_a_patch
Comment 28 Lupo 2012-06-06 22:23:11 UTC
(In reply to comment #27)

Sumana, I think it's unlikely Ahmad will reply. His last contribution on bugzilla seems to date back to July 2009, and, assuming he is User:AhmadSherif, he appears to be inactive at the Arabic Wikipedia (apparently his home wiki) since September 2010.
Comment 29 Lupo 2012-06-06 22:26:06 UTC
In any case, it appears Aaron had begun implementing this back in 2008. At
least he added a user_last_timestamp column to table user_newtalk (see r33520
and two follow-ups r83137 and r854185). But that appears to be all; I don't see
that Db field used anywhere.

With this field present, it is, however, relatively simple to implement the
feature requested here. (Although the name is a bit awkward: one cannot store
the time the user last viewed his or her talk page there, as that's when the
row is deleted, but it needs to store the timestamp of the first revision made
after the user last saw his or her talk page.)

But given that nothing happened for 3 - 4 years on this issue, I'd like to have
some input from Aaron or Roan as to whether that is still desired. If so, I
could invest some time to do this.

Also, there's two issues here on which I'd like to have Roan's or Aaron's
input:

- First, counting revisions as "number of messages" as Ahmad did in his patch
strikes me as patently incorrect. Consider the same editor amending his last
message: that's two revisions, but one message. Or a vandal editing a talk page
and getting reverted: 2 revisions, zero messages. So I'd prefer to not attempt
counting revisions or messages at all, but just go for the plural in the "(last
change)" part, which then becomes "(last changes)" and a diff to the correct
revision, as originally suggested.

- Second, Roan said in comment 19 that there was only one row per user talk
page in table user_newtalk. That may have been the intention (the code does an
INSERT IGNORE), but that's not what really happens. The table gets a new row
for each revision. The reason is, I think, that the table is lacking a primary
key; it should have a PRIMARY KEY (user_id, user_ip). I would suggest adding
such a primary key.
Comment 30 Aaron Schulz 2012-06-07 04:54:36 UTC
(In reply to comment #29)
> But given that nothing happened for 3 - 4 years on this issue, I'd like to have
> some input from Aaron or Roan as to whether that is still desired. If so, I
> could invest some time to do this.

The lack of a PRIMARY KEY can be worked around. I put up a DRAFT change in gerrit for this. If you a gerrit/ldap account, I can make it visible to you to work on. It still needs polish/i18n tweaks.
Comment 31 Lupo 2012-06-07 05:57:39 UTC
(In reply to comment #30)
> I put up a DRAFT change in
> gerrit for this. If you a gerrit/ldap account, I can make it visible to you to
> work on. It still needs polish/i18n tweaks.

Yes, I do. Please make it visible for me, then, and I'll go take a stab at this.
Comment 32 Dereckson 2012-06-10 17:26:59 UTC
Code review: Gerrit change #10545

Bug assigned to patch submitter.
Comment 33 Rob Lanphier 2012-06-22 18:36:53 UTC
Hi Niklas, assigning to you as a possible review task.  Feel free to unassign yourself if you don't think the assignment is appropriate.  Aaron's not the best person to review since this was originally his code.
Comment 34 Liangent 2012-08-10 14:33:29 UTC
Already merged.
Comment 35 Siebrand Mazeland 2012-08-10 17:19:27 UTC
Looks like I missed this patch set before it was merged. The current solution suffers heavily from patchwork disease. It's bound to be pretty much impossible to translate in some languages.

Could this be rewritten to use only 2 messages instead of the current 4.

Current:
# You have $1 from {{PLURAL:$3|another user|$3 users}} ($2).
# You have $1 from many users ($2).
# {{PLURAL:$1|a new message|new messages}}
# last {{PLURAL:$1|change|changes}}

Should be:
# You have [$1 {{PLURAL:$2|a new message|new messages}}] from {{PLURAL:$3|another user|$3 users}} ([$4 last {{PLURAL:$1|change|changes]}}).
# You have [$1 {{PLURAL:$2|a new message|new messages}}] from many users ([$4 last {{PLURAL:$1|change|changes]}}).

Parameters:
* $1: link to user talk page
* $2: number of talk page edits since last read
* $3: number of unique editors of user talk page since last read
* $4: link to diff for talk page (last visited - to current)

The message probably has to be wrapped in a span so that the external link icon doesn't pop up.
Comment 36 Platonides 2012-08-10 18:53:54 UTC
Also, in this case I would have reused newmessageslink and newmessageslinkdiff (despite the fact that it is generally better to create new ones), since the context is the same, and it's preferable to get a non-pluralised message in your language that a patchset one with plural support. In this case, it's backwards compatible..
Comment 37 matanya 2012-08-11 23:36:12 UTC
=Patch by Siebrand to revert this:

https://gerrit.wikimedia.org/r/#/c/18482/
Comment 38 Umherirrender 2012-09-25 19:00:02 UTC
(In reply to comment #37)
> =Patch by Siebrand to revert this:
> https://gerrit.wikimedia.org/r/#/c/18482/

Is now abandoned, closing this bug again

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links