Page MenuHomePhabricator

[Task] Make hover cards work on property links
Open, NormalPublic

Description

Hover cards don't currently work on links to property pages. They should.

The reason seems to be that the property namespace is not a content namespace. Are there any reasons not to make it a content namespace?

Event Timeline

Lydia_Pintscher raised the priority of this task from to Normal.
Lydia_Pintscher updated the task description. (Show Details)
Lydia_Pintscher added subscribers: Bene, Lydia_Pintscher, hoo.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 18 2015, 7:59 AM
aude added a subscriber: aude.Jun 18 2015, 8:34 AM

no reason not to, in my opinion

hoo added a comment.Jun 18 2015, 8:46 AM

I can't think of a reason not to do this... in the past I explicitly added the Property: namespace to the namespaces being searched per default, but didn't turn it into a content namespace (which would also make it searched per default). I can't remember whether we did that for a specific reason or just because it was less intrusive to do so.

Bene added a comment.Jun 18 2015, 8:59 AM

Just if someone wonders, hovercards work on properties again because I've manually added them. While this is a reasonable fix, we should still consider to make the property namespace a content namespace.

Properties are like templates or pages in the MediaWiki: namespace. They are not "content" in the same sense as Items are content. Properties are used to describe Items. I do have very mixed feelings about moving Properties up to the same level as Items. Please fix hovercards (a.k.a. Popups extension) instead.

Our example configuration contains no default setting for this. This means, if your repo is set up to use the Item: namespace prefix, this will not be a content namespace.

@Bene and I discussed this during the hackathon and came to the conclusion that certain changes need to be made in the way new renderers are registered. One of the architectures we discussed removed the need to make any changes to Properties.

I am yet to submit a patch with these changes and hope to get it done by July.

Change 232236 had a related patch set uploaded (by Prtksxna):
Check renderers' matcher method to find the appropriate renderers for a link

https://gerrit.wikimedia.org/r/232236

matej_suchanek set Security to None.
Bene renamed this task from make hover cards work on property links to [Task] Make hover cards work on property links.Sep 2 2015, 6:22 PM
Jdlrobson changed the task status from Open to Stalled.Sep 28 2015, 6:36 PM
Jdlrobson added a subscriber: Jdlrobson.

This appears to be stalled.

hoo changed the task status from Stalled to Open.Oct 1 2015, 8:58 AM

Thanks @hoo open questions on patch so we can get this wrapped up.

Jdlrobson changed the task status from Open to Stalled.Oct 9 2015, 12:53 AM
hoo changed the task status from Stalled to Open.Oct 12 2015, 12:27 PM

The change is still up for review and it seems like there is an example making use of the functionality at https://gerrit.wikimedia.org/r/232237.

It's probably best to keep this task open so that we don't loose sight of this, although reviewing the patch might be stalled for you (which doesn't necessarily mean it is stalled for everyone; @Bene might also want to review it).

FWIW stalled tasks can easily be tracked rather easily in such a way that you do not lose sight of them. See https://phabricator.wikimedia.org/dashboard/view/125/ to get an insight into our workflow.

Right now patch to review tells me my team that we need to review something/give something attention, but this is not true. It seems "Blocked-on-Wikidata" might be a better way of filtering this out from the queue - I need an example use of functionality from your team to assert that this patch does what is needed to support your use case. Once I have that I'll happily merge.

example functionality: go to https://www.wikidata.org/wiki/Q1 and hover over "significant event" and "big bang". Hover cards should show up for both (links to other items and links to properties).

I understand what you are trying to achieve and what the desired outcome is.
I just want to know whether https://gerrit.wikimedia.org/r/232236 helps you do that.
This requires you building something.

To be clear I expect the Wikidata team to make use of https://gerrit.wikimedia.org/r/232236 to make this happen. Once I see working code I'll merge https://gerrit.wikimedia.org/r/232236 otherwise it will not happen.

Se4598 added a subscriber: Se4598.Oct 13 2015, 12:00 AM

I understand what you are trying to achieve and what the desired outcome is.
I just want to know whether https://gerrit.wikimedia.org/r/232236 helps you do that.
This requires you building something.
To be clear I expect the Wikidata team to make use of https://gerrit.wikimedia.org/r/232236 to make this happen. Once I see working code I'll merge https://gerrit.wikimedia.org/r/232236 otherwise it will not happen.

I don't like that attitude. @Prtksxna already uploaded a example renderer (needs probably rebase/update) at https://gerrit.wikimedia.org/r/232237 as hoo already pointed out, so do your QA check for 232236. Wikidata-Team isn't the only one who in the end can use it. The change will eventually also open the possiblity for onwiki community user scripts/renderer.

Something not covered by the patch, which isn't a problem, is that after registering the renderer you may have to manually enable popups for all wanted non-content namespace links, but which can done with a following mw.popups.removeTooltips( $elements ); mw.popups.setupTriggers( $elements );

Also if in hindsight it turns out there's missing something, then let's prepare another patch. That isn't impossible, right? At the moment you're also working against and blocking volunteers/volunteer work...

I'm pissed off.

Apologies for being pissed off but please as always
assume good faith. My team has to manage various extensions and I'm trying to set expectations and clear out a backlog of patches. Sadly written communication doesn't always come across as it should. I should be pissed off given my team has been told to work on a bunch of extensions it has had no prior involvement in but I'm trying to manage this as best as I can.

I have reviewed the patch in question from @prtskana but it would be irresponsible for me to merge it until something consumes it and ensures its future proofness and quality, so no let's get this right first time rather than merge it just in case. YAGNI [1]

No one was even bothering to review it so to be honest it's not quite fair to give me stick for caring and giving this attention. My asks are not huge here.

That patch you point to says "do not merge". I have no idea if it solves the problem referred to in this card. The patch in question has no tests and the author is not working on hovercards so sorry but you're being unfair here too.

Since the bug refers to wikidata anyone involved in the wikidata project would be a natural candidate to work on this given they know that code well.

By all means if a gadget is dying to make use of it please show me one so I can test this and prove it's usefulness.

[1]https://en.m.wikipedia.org/wiki/You_ain%27t_gonna_need_it

Ok, I try to explain what this patch aims at.
The patch in question solves the problem that previously it wasn't possible to add new renderer without overwriting popups javascript functions with own customized ones, which are prone to break if the original function changes. Also obviously this only works easy with one external party.
You can see that approach taken at the wikidata gadget by bene* at https://www.wikidata.org/wiki/MediaWiki:Gadget-PopupsFix.js who overwrites mw.popups.render.article.init.

Now with the change all you have to do is to add your own renderer to a js-object provided by popups. No overwriting of popups code needed!

As practical working thing, I just created https://www.mediawiki.org/wiki/User:Se4598/testpopups.js
There I first replaced the popups function by the one in the relevant changeset (to create the after-merge-state).

Then I created a own renderer (code from 232237, slightly modified). The matcher-function returns true in this example, but operates on the URL (contrary to what 232237 states), so here Wikidata can inspect the href of the link and extract the namespace etc.).

Testing: include my testscript, see all appearing hovercards in blue. (Only works on content-namespace links, as previously stated, but can be changed without modifing popups code)

Benefit: The already existing gadget could be modified to not overwrite popups code any longer, so no dirty hacks and no maintenance required if mw.popups.render.article.init needs to change! :) This would lay the foundation that own renderer code could be included in Wikidata itself.

@Jdlrobson Sorry, I understand you. I hope you now understand what the intention behind that patch is and this is answerred: "I just want to know whether https://gerrit.wikimedia.org/r/232236 helps you do that."

PS: A thing where I'm not sure about if this is the right thing is the argument supplied to the matcher-function. Currently it is the href-part of a link, e.g. "/wiki/Mainpage".
But the gadget works/inspects on the title-attribute of the link. Theoretically that could be modified to work on the url, but maybe there are edge cases, which require addition infos.

There was once a bug with chinese and language variants where title and url would yield different page titles, but I'm not able to judge if that is relevant for a matcher-function here, probably only in rare edge cases, which never emerge.

PS: A thing where I'm not sure about if this is the right thing is the argument supplied to the matcher-function. Currently it is the href-part of a link, e.g. "/wiki/Mainpage".
But the gadget works/inspects on the title-attribute of the link. Theoretically that could be modified to work on the url, but maybe there are edge cases, which require addition infos.

The title attribute is unreliable, instead mw.popups.getTitle() should work easily for Wikidata popups too.

Thank you @Se4598 I will take a look at the
Gadget today. I didn't realise there was an existing one so that certainly adds more urgency/clarity to this.

I hope to take a look today and hopefully have time to add a follow up patch with tests.

Definitely would be more preferable to get it into wikidata with some tests though after that due to the lack of discoverability of these gadgets.

Change 232236 merged by jenkins-bot:
Check renderers' matcher method to find the appropriate renderers for a link

https://gerrit.wikimedia.org/r/232236

Prtksxna removed Prtksxna as the assignee of this task.Jan 3 2018, 4:37 AM