tag: Add untag functionality

[siddharthvp]

The code is quite complicated due to the asynchronicity of the API call to find the redirects to templates, whose main form wasn't found on the page. This required almost a overhaul of Twinkle.tag.callbacks. UPDATE: Code is now much simpler after I discovered that it's possible to get linkshere of multiple pages with a single API call.

Since existing tags are detected by parsing the HTML code using jQuery, untagging is only supported if Tag module is invoked while viewing the current version (either in Read mode, or a diff involving the current version, or a permalink page to the current version).

New additions to Morebits.js:

• HTMLFormElement.prototype.getUnchecked - just a copy of HTMLFormElement.prototype.getChecked, but gives the unchecked checkboxes of a given name, rather than checked ones.
• Morebits.pageNameRegex - given a string foobar or Foobar, returns [fF]oobar, which can then be used to construct a regex expression.

I will think about adding additional features later, like:

• folding existing tags into {{multiple issues}} along with the tags being added Done
• untagging of custom tags
• untagging of rcat templates on redirects

Tester-friendly version is at User:SD0001/friendlytag.js, which differs from the file here in that

• works in userspace, feel free to test here.
• additions to morebits.js are available in this file itself.
• Twinkle.tag(); at the end
• lots of console.log's
• 1-second setTimeout after page load to prevent crazy Morebits/Twinkle undefined errors

[siddharthvp]

I've just added the feature to group existing tags into the MI template. Closes the antique issue #13.

All the changes have been extensively tested (I've raked up my enwiki userspace edit count by quite a bit).

This is a gonna be very useful, so can someone review this? I have also updated the testing version at User:SD0001/friendlytag.js; also wrapped its contents into a 1-second setTimeout to prevent those crazy "Morebits is not defined" or "Twinkle is not defined" errors.

[Amorymeltzer]

I'd like to start reviewing this (and the others), but it's very hard to go through something with a dozen listed commits, some of which appear unrelated to the task and some which could've been squashed, without at least some more guidance on what you're trying to do and where. You've got merge conflicts from your last changes to the tag module, are you planning on revisiting these?

[siddharthvp]

Thank you for reviewing this. Given the extent of changes (the Twinkle.tag.callbacks for articles is almost completely overhauled), I think it'd be fruitless to review this by looking at diffs. Most of the intermediate commits need not be looked at as they're just WIP stages (feel free to squash in all commits while merging). I have tried to make the code well-commented, though it's still admittedly difficult to follow because of the non-linear flow of control. I think it'd be best if you only look at the final code and test it out (which I have already done).

I have resolved the merge conflicts (though unfortunately have not tested after doing this).

[Amorymeltzer]

Given the extent of changes (the Twinkle.tag.callbacks for articles is almost completely overhauled), I think it'd be fruitless to review this by looking at diffs.

Oof, yeah. I've held off looking at #487 and #488 with the idea to do all your tag-related things in order to avoid extra work on your part, but looking at them now, it seems that isn't the case. As far as I can tell from a quick look, those are each going to have a ton of conflicts with this; in fact, it seems it'd be easier to layer this on top of those rather than the other way around? There are some other changes I've held off with the intent of getting through these, but I'd like to.

I'm a bit busy this coming week but I will try to review #487 and #488 within the next 10 days or two, with the goal of getting those and #541 and #536 merged (I'm personally opposed to #488 but I think I'm in the minority there) before the next on-wiki update (early April). After that, the decks should be cleared tag-wise, so then if you can deal with the inevitable merge conflicts here, I'll take the time to go through this with the care it deserves. How does this sound for a plan forward?

[siddharthvp]

Go ahead and merge #487, #536 and #541. I should be able to fix the arising conflicts easily. #488 is still sketchy and maybe requires some discussion so let's hold back on that.

[Amorymeltzer]

Ugh, one more thing: Twinkle.tag.callbacks.article.main (L1201) doesn't exist! Tagging for {{merge}} thus dies rather crankily. Twinkle.tag.callbacks.main got renamed to ``Twinkle.tag.callbacks.article` but

Also, translationListPage is sandwiched between Twinkle.tag.callbacks.redirect and Twinkle.tag.callbacks.file; should probably be moved either into or after Twinkle.tag.callbacks.article, depending on how the above is handled.

[siddharthvp]

Moved translationListPage into callbacks.article, within pntPage.load(...)

fwiw also moved Twinkle.tag.multipleIssuesExceptions to below the article tags list, as multiple issues exceptions are only for article tags.

Something like // the ability to remove tags accurately relies on these being capitalized to match the class applied to the ambox template

Actually, this isn't the case. Tags can be removed as long as the "Template:" + ambox class name is same as the template page name, or redirects to it. The capitalisation/correctness of the templates listed has no role to play in removing. That being said, there was no reason to make tags uppercase at the end of the day. Self-trout. Let me see if I have time to revert it back to the good old lowercase.

[siddharthvp]

I was nearly three-fourths my way towards converting everything back to ucfirst, when I realised that keeping them ucfirst has an advantage -- it leads to better consistency! If we list available tags as lcfirst, it makes sense to list the already present tags too in lcfirst form. But the trouble is that all tag templates have their |name= param in ucfirst form. We could easily convert them to lcfirst by using Morebits.string.toLowerCaseFirstChar, or can we? while box-Advert is easily understood to mean advert, we would trip up on box-POV which would be interpreted as pOV. There are 2 solutions to this: (i) look at the second char - if it's uppercase, skip toLowerCaseFirstChar. So that Prose becomes prose since r is lc but POV remains POV as O is uc (a kludgy hack!), or (ii) edit all templates on-wiki to make their names lcfirst, except in case of POV or GOCEInuse (laborious, most of these are under template-protection, so I can't do this at all).

Simpler alternative is keep the tags in ucfirst form. Since I had already done most of the work in making them lcfirst and still have that work in git stash, @Amorymeltzer I will leave it to you to decide which way we go. I don't have a preference really.

[Amorymeltzer]

I won't be able to review until tomorrow, but wanted to reply to this to head-off confusion.
Sorry for causing it! My first comment was unclear, when I suggested ability to remove tags accurately I was trying to write a short comment for later reviewers considering adding templates. As you (of course) correctly note:

Tags can be removed as long as the "Template:" + ambox class name is same as the template page name, or redirects to it.

Where it does matter though (at least as of my last review) is in removing already-present templates from the addable list. That is, if {{autobiography}} is present, this code detects the ambox class Autobiography and removes Autobiography from our list of templates; if we have the list capitalized, that works, but if the list uses lowercase, it won't. That was why I had initially suggesting not changing the capitalization but instead parsing them all with a ucfirst (as you note for cases like POV, we can't really do the alternative of lcfirsting the ambox templates). As I said above, I don't really care anymore — the only reason to insist not changing them from the original was to keep style with other TW modules, but it doesn't really matter and I don't really think it's worth adding in some code to ucfirst the list.

I think what your latest comment is saying is that kind of agreed and thus went to try to undo (e.g. undoing the cleanup->Cleanup back to cleanup) the changes (as first mentioned here but ran into issues (especially around POV)? I say don't make any (more) changes, i.e. keep the cleanup -> Cleanup change: it takes care of the removal issue alluded to above, it matches the classes used in {{ambox}}, it saves some complicated-ish code, and matching Twinklecase is laudable but shouldn't be the end-all-be-all.

Cool? Sorry for the all the back-and-forth on this...

[Amorymeltzer]

@siddharthvp I can try and run this down tomorrow, but if you have a moment beforehand: there's a typeerror at L1583 (params.tagsToRemain.forEach) when attempting to tag with merge, params.tagsToRemain is undefined. It freezes when trying to retrieve the talk page (Tagging other page).

[siddharthvp]

Where it does matter though (at least as of my last review) is in removing already-present templates from the addable list.

Ooof, yeah! I had forgotten about that aspect. Sorry for causing the confusion, I had a good role to play in that :)

[Amorymeltzer]

What a slog!

[Amorymeltzer]

@siddharthvp As far as I can see everything checks out, so if you've got nothing further, dare I say that we're all set?

[siddharthvp]

@Amorymeltzer Yes!

[Amorymeltzer]

💯 🏆 💯

Thanks for sticking with it so long!

[siddharthvp]

Great! This pull has been a great learning experience, at least in terms of JS programming. The functions for removing tags and re-placing tags into MI - I had written 3 times, each time discovering a superior programming technique, though the end result was the same. Thanks for reviewing this. So, you're going to deploy to the wiki now, or with the June update?

[Amorymeltzer]

I'll put it up in June. If you can think of good places to advertise this change, I think it'll be popular!