Last modified: 2007-07-11 19:22:01 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 "(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.
Is the current version of the field used anywhere? If so, for what?
(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.
Apparently this has been implemented in some form at 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).
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
- Jane edits User talk:Bob. wl_notificationtimestamp is set to the current
- 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)
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.
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.
(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
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.
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.
(In reply to comment #8)
> 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.
Could the attached patch be reviewed and applied to the trunk?
(In reply to comment #10)
> Could the attached patch be reviewed and applied to the trunk?
Well, could it?
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?
(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.
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.
Done in r23575.
(In reply to comment #15)
> Done in r23575.
Looks good to me. Thanks
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.