ayg wrote:

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:

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).

Created attachment 3793
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)

Attached:

Created attachment 3794
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.

Attached:

ayg wrote:

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
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.

ayg wrote:

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)

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.

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?

ayg wrote:

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.