Page MenuHomePhabricator

Watchlist and recent changes should be able to show diffs inline
Open, MediumPublic

Description

Watchlist and recent changes should be able to show diffs inline.

I mean, it's 2013. Seriously.


Version: 1.22.0
Severity: enhancement

Details

Reference
bz51958

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:08 AM
bzimport added a project: MediaWiki-Watchlist.
bzimport set Reference to bz51958.
bzimport added a subscriber: Unknown Object (MLST).

Changing to "Enhancement".

It exists as a Gadget. See [[Wikipedia:Tools/Navigation popups]]

Or at least it should allow loading the diffs using AJAX. Navigation popups is rather ugly: 1) The timeout before it sends the request 2) you can't move your pointer away or the popup closes.

https://en.wikipedia.org/wiki/User:Writ_Keeper/Scripts/inlineDiffDocs does this.

To test:
importScript('User:Writ Keeper/Scripts/commonHistory.js');

I think this could be a good new gadget (for now, and possibly a mediawiki feature in future months/years); I shall suggest that on the talkpage, next.

It doesn't seem to work with enhanced watchlist/RC :(

(I definitely like what it's doing when it works, though, I was imagining pretty much exactly that.)

writ.keeper.enwp wrote:

What doesn't it work with? Seems to work on my watchlist and RC on en at least; is there a different thing for which it doesn't work?

(In reply to comment #6)

What doesn't it work with? Seems to work on my watchlist and RC on en at
least;
is there a different thing for which it doesn't work?

If you "bundle" changes, (by enabling in preferences:
[Recent change]->"Group changes by page in recent changes and watchlist"
and
[Watchlist]->"Expand watchlist to show all changes, not just the most recent")
then it doesn't work.

writ.keeper.enwp wrote:

Well, jeez, it's hardly my fault that, if you switch the grouped RC changes on, it changes the format for both RC *and* watchlist to use tables instead of uls. (Is that a bug, by the way? Why would changing an RC setting also change the format of the watchlist too? Switching on the expanded watchlist without switching on the grouped RC doesn't change the format of the watchlist page, and the script still works.) Anyway, I'll fix that.

writ.keeper.enwp wrote:

Ah, nevermind, I see the point now. Still going to try to fix it.

writ.keeper.enwp wrote:

Okay, done! I think it should work on the enhanced RC and watchlist now, for both collapsing and uncollapsing entries. Give it a shot and let me know if it works, and whether there are any more suggestions.

Works and I like it :)

If we wanted to make this a core feature, I think we should just make
it hook to the "N changes" / "cur | prev" links instead of adding more
(as the watchlist interface is already rather cluttered and
intimidating).

When we do that, we should make viewing the diff inline update the
"page visitedness" status (whether the pages on Watchlist are bolded;
I think there's some default gadget at en.wp which disables this
incredibly useful feature :(, so you'd have to disable it to see the
result there).

If we were to merge your script into core MediaWiki, we'd have to make
it adhere to our coding conventions (see [[mw:CC]] and [[mw:CC/JS]])
and you'd have to agree to a relicense it under GNU GPL or a
compatible license (e.g. MIT) – unfortunately CC BY-SA (under which
you implicitly released it by putting it in a Wikipedia page) is
apparently not compatible, but things can easily have multiple
different licenses.

As for general review of the code, if you're interested :)

  • After you click the [inspect diff] link, there's a period of time when nothing is happening (while the API request is loading) – there needs to be some indicator that the script is not stuck. I pastebinned a simple solution here: http://pastebin.com/cb7W2U9i , feel free to use it.
  • Why are you allowing only one API request to be active at a time?
  • Selectors like ":contains" are rather slow and should not be used unless absolutely necessary; not sure if this is the cause, but the script can take a few seconds to set up the buttons on large pages
  • Changing the 'type' of <input>s and <button>s after they are created will throw an exception on IE<=8; it's better to use something like $('<input type=button>')[0] to create them
  • Using Object.keys will likewise throw an exception on some older browsers, I think.
  • We have a few fancy libraries you could use instead of reimplementing some logic, like mediawiki.api or mediawiki.Uri, or the aforementioned jquery.spinner. They all are (or should be) documented at https://doc.wikimedia.org/mediawiki-core/master/js/ and can be loaded using mw.loader.using([dependencies], callback) like in my pastebin above.
Rillke added a comment.Dec 8 2013, 6:34 PM

(In reply to comment #11)

$('<input type=button>')[0] to create them

Note that this should be valid HTML then, including closing tags for elements that need closing tags.

Just FYI, here's a screenshot of Writ Keeper's script in action: