Last modified: 2007-07-11 19:22:01 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 10277 - Watchlist notification timestamp stores timestamp of last edit rather than timestamp of last visit
Watchlist notification timestamp stores timestamp of last edit rather than ti...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
History/Diffs (Other open bugs)
1.11.x
All All
: Normal enhancement (vote)
: ---
Assigned To: Brion Vibber
: patch, patch-need-review
Depends on:
Blocks: 10292 10291
  Show dependency treegraph
 
Reported: 2007-06-15 16:30 UTC by Roan Kattouw
Modified: 2007-07-11 19:22 UTC (History)
2 users (show)

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


Attachments
Prevents wl_notificationtimestamp from being re-set if it's already set; prevents unnecessary NULLifying (1021 bytes, patch)
2007-06-16 19:00 UTC, Roan Kattouw
Details
Shows the effects of previously attached patch (18.12 KB, image/png)
2007-06-16 19:03 UTC, Roan Kattouw
Details

Description Roan Kattouw 2007-06-15 16:30:51 UTC
I have discovered the following:

- Bob adds User talk:Bob to his watchlist. wl_notificationtimestamp is set to NULL.
- Jane edits User talk:Bob. wl_notificationtimestamp is set to the current time.
- John edits User talk:Bob. wl_notificationtimestamp is set to the current time.
- Bob views User talk:Bob. wl_notificationtimestamp is set to NULL.
(for those who wonder: wl_notificationtimestamp is a database field in the watchlist table)

This means wl_notificationtimestamp stores the timestamp of the last unseen edit (in this case John's) rather than the timestamp of the last time Bob viewed the page. The latter not only makes more sense to me, it would also allow for some nice new features (see below).

I propose this:

On the short term:
- Change User.php so that when a user views a watched page, wl_notificationtimestamp is set to the current time (rather than NULL).
- Change UserMailer.php so that when a watched page is edited, wl_notificationtimestamp is unharmed.

On the long term:
- Replace the wl_notificationtimestamp field with a wl_lastseenrev field which stores the last viewed revision ID.

The benefits:
- The "(changed since last visit)" text in the history can be replaced by a "(last visit)" text which is placed next to the revision you've last seen. This way you can easily see which edits have been made since your last visit.
- Add a "This page has been changed since your last visit." banner on top of a watched page, with a "view changes" link which shows all changes since the last visit.
- The wl_lastseenrev field will reduce the amount of times the watchlist table needs to be updated.

Now I can go ahead and write a patch for all of this (except for the lastseenrev field), but I first wanted to now whether this idea is supported by the community. Please give your opinion here on whether this should be done or not.
Comment 1 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-06-15 22:32:14 UTC
Is the current version of the field used anywhere?  If so, for what?
Comment 2 Roan Kattouw 2007-06-15 23:06:53 UTC
(In reply to comment #1)
> Is the current version of the field used anywhere?  If so, for what?
> 

Yes. It's used to:
- check if the mail for "this page has been changed should be sent"
- check if the (changes since last visit) message in the history should be displayed
- bolding pages in Special:Watchlist
- sorting pages in Special:Watchlist

Although all of those would still be possible after the change.
Comment 3 Roan Kattouw 2007-06-16 12:10:55 UTC
Apparently this has been implemented in some form at EnotifWiki:

http://www.wikipage.org/wikinaut/index.php/Enotif/Differences_between_MediaWiki_and_EnotifWiki

although I intend to also place a banner above a changed watched page (i.e. when you visit it, you'll see a banner "This page has been changed since your last visit". EnotifWiki doesn't implement this).
Comment 4 Roan Kattouw 2007-06-16 19:00:25 UTC
Created attachment 3793 [details]
Prevents wl_notificationtimestamp from being re-set if it's already set; prevents unnecessary NULLifying

On a second thought, it may be better to leave wl_notificationtimestamp as it is, with the exception that it's only updated when the page is first edited. The attached patch does this. To illustrate this with the example I used before:

- Bob adds User talk:Bob to his watchlist. wl_notificationtimestamp is set to
NULL.
- Jane edits User talk:Bob. wl_notificationtimestamp is set to the current
time.
- John edits User talk:Bob. wl_notificationtimestamp is NOT CHANGED.
- Bob views User talk:Bob. wl_notificationtimestamp is set to NULL.

Because John's edit (and any subsequent edit) doesn't change the timestamp, it'll reflect the time of Jane's edit until Bob views the page again. The wl_lastseenrev field won't be needed, as that doesn't really optimize anything.

The only visible effect of this patch is that in the page history, ALL changes since your last visit will get the green "updated since my last visit" mark, rather than just the last change (screenshot will follow).

As I said before, this patch (which fixes the initial bug) allows for some nice features. Since they require messing around with skins and all that, I'm not gonna code them, so I've filed separate bugs for them:

- Adding a "changes since last visit" link in the history of a watched page and in Special:Watchlist. See also bug 10291
- Adding a banner on top of a changed watched page saying "This page has been changed since your last visit" with "changed" linking to all changes since the last visit. See also bug 10292

If and when this patch is merged into the trunk, this bug can be closed, even if the aforementioned features haven't been implemented yet (as they have their own bugs now)
Comment 5 Roan Kattouw 2007-06-16 19:03:30 UTC
Created attachment 3794 [details]
Shows the effects of previously attached patch

The attached image shows how the previously attached patch changes the history.

One thing I forgot to mention: the previously attached patch also contains a minor optimization for User.php: when a user views a watched page that hasn't changed, the code won't try to set the timestamp to NULL if it's already NULL.
Comment 6 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-06-17 01:57:35 UTC
That sounds reasonably sane.  I'm not sure why you would want a field that contains the time of the last edit, given that that's presumably in page already.  Of course, someone else needs to look this over and test it.
Comment 7 Roan Kattouw 2007-06-17 08:04:25 UTC
(In reply to comment #6)
> I'm not sure why you would want a field that
> contains the time of the last edit, given that that's presumably in page
> already.

It doesn't always store the time of the last edit. It stores the time of the last edit if the user watching hasn't seen it yet, or NULL if he has.

Comment 8 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-06-17 08:11:37 UTC
Right, but then you may as well make it a boolean.  Your change (stores the *first* edit you haven't seen) makes much more sense, which was my point.  But perhaps it would be more useful still to have it store the revision id of the first edit you haven't seen?  Wouldn't that make the diff generation simpler?  I guess it doesn't make much difference, though.
Comment 9 Roan Kattouw 2007-06-17 08:18:45 UTC
(In reply to comment #8)
> But
> perhaps it would be more useful still to have it store the revision id of the
> first edit you haven't seen?  Wouldn't that make the diff generation simpler? 
> I guess it doesn't make much difference, though.

It would make diff generation simpler, but it makes finding out whether the page has changed since your last visit more expensive: you have to see it the stored revid is the page's current revid (requires looking it up in the page table or wherever that's stored), whereas with the timestamp you can just check if it's NULL. For displaying the green things in the history it doesn't matter, since you have to get all revids and timestamps from the DB anyway. Also, with the lastrev approach, the watchlist table would have to be updated when someone's editing their own page, whereas that's not necessary with the timestamp approach.

Comment 10 Roan Kattouw 2007-06-18 18:35:16 UTC
Could the attached patch be reviewed and applied to the trunk?
Comment 11 Roan Kattouw 2007-06-24 20:01:32 UTC
(In reply to comment #10)
> Could the attached patch be reviewed and applied to the trunk?
> 

Well, could it?
Comment 12 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-06-24 23:11:44 UTC
I don't feel I'm clear enough on what all this does to commit.  Brion, sorry to impose, but would you care to take a look?
Comment 13 Roan Kattouw 2007-06-28 12:58:13 UTC
(In reply to comment #12)
> I don't feel I'm clear enough on what all this does to commit.  Brion, sorry to
> impose, but would you care to take a look?

It really does very little. All the user notices is multiple "updated since my last visit" texts in the page history (see previously attached screenshot), and all that changes internally is how the wl_notificationtimestamp in the watchlist table is interpreted: it used to be the timestamp of the *last* edit the user hasn't seen, this patch makes it the timestamp of the *first* edit the user hasn't seen. If the user has already seen the current revision, the field is set to NULL in both cases.
Comment 14 Aaron Schulz 2007-06-30 01:00:25 UTC
1) As for the diff, then line 1845 change seems redundant, that is "set to NULL where it is not NULL", either way it's the same result.

2) This is only on if $wgUseEnotif is on, that is the two functions that NULL the value or set a timestamp. Perhaps the idea was that if you were sent an email for the page/diff/time for each new edit, you would know about it. In that sense "Since your last visit" would still be a bit misleading, because either you viewed the page/diff here or not.

Given that, I suppose the first change is fine. Rather than just the latest edit you didn't see having a notice, they all would.
Comment 15 Aaron Schulz 2007-06-30 01:03:54 UTC
Done in r23575.
Comment 16 Roan Kattouw 2007-06-30 07:50:35 UTC
(In reply to comment #15)
> Done in r23575.
Looks good to me. Thanks
Comment 17 Brion Vibber 2007-07-11 19:22:01 UTC
r24017 fixes a regression from r23575 -- wl_notificationtimestamp didn't get updated anymore on update notification, leading to duplicate mails being sent out.

Doing a WHERE on x=NULL doesn't work; need to do 'x IS NULL' here.
Perhaps we should make the DB functions smarter to handle that case to do what we'd expect it to do (as we do turning arrays into 'IN (...)') but this'll fix it for now.

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


Navigation
Links