Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for DISPLAYTITLE caption #1410

Merged
merged 1 commit into from Feb 29, 2016
Merged

Support for DISPLAYTITLE caption #1410

merged 1 commit into from Feb 29, 2016

Conversation

@mwjames
Copy link
Contributor

@mwjames mwjames commented Feb 17, 2016

Adds general support for subjects that contain a {{DISPLAYTITLE:Foo}}. The pre-defined property _DTITLE is unrestricted therefore manually annotations via Display title of [0] are allowed.

Settings in connection with {{DISPLAYTITLE:Foo}}:

$wgAllowDisplayTitle = true;
$wgRestrictDisplayTitle = false;

[0] https://www.semantic-mediawiki.org/wiki/Help:Special_property_Display_title_of

@mwjames mwjames added this to the SMW 2.4 milestone Feb 17, 2016
@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 17, 2016

@kghbln @cicalese Since we support DEFAULTSORT, I now made this a bit more round in the sense that if a subject contains a DISPLAYTITLE then a Display title of property will be set and wikipage captions are to recognize the explicit title and modify the output in queries etc. through the wikipage dataValue object.

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 17, 2016

@cicalese I made a comment in the serializer [0] on how to access the display title content but I'd like to see some performance tests whether this is elevating the runtime or not before making this available.

[0] https://github.com/SemanticMediaWiki/SemanticMediaWiki/pull/1410/files#diff-45f5fa9530bfcefe0517e74d8acf8a3eR102

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 17, 2016

@SemanticMediaWiki/testers Anyone care to test?

@cicalese
Copy link
Contributor

@cicalese cicalese commented Feb 17, 2016

Thank you, @mwjames!

Coincidentally, I was working on my older patch tonight, but this is obviously a more full featured solution. I will test this, with the QueryResultSerializer bit uncommented plus my suggested changes to res/smw/data/ext.smw.data.js and res/smw/data/ext.smw.dataItem.wikiPage.js to make this useful for JavaScript result formats.

@cicalese
Copy link
Contributor

@cicalese cicalese commented Feb 17, 2016

I'm having the following issue while testing. I create a page with DISPLAYTITLE set.

screen shot 2016-02-17 at 7 53 50 am

I save the page and immediately browse the properties for the page and see the "Display title of" property:

screen shot 2016-02-17 at 7 54 02 am

But, when I refresh the page, it disappears:

screen shot 2016-02-17 at 7 54 18 am

This is probably not significant, but if I refresh the page again, "Modification date" changes from text to a link:

screen shot 2016-02-17 at 7 54 31 am

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 17, 2016

I tried to follow your example but could not confirm the observation. Did you run a php update.php (2.4 requires to run it)?

Tested with MediaWiki 1.25.1
PHP 5.6.8 (apache2handler)
MySQL 5.6.24

@cicalese
Copy link
Contributor

@cicalese cicalese commented Feb 17, 2016

Yes, I ran update. My environment:

MediaWiki 1.27.0-alpha (86c08b2) 18:39, 16 February 2016
HHVM 3.6.5 (srv)
MySQL 5.5.47-0ubuntu0.14.04.1

I'm running under MediaWiki-vagrant.

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 17, 2016

@kghbln In case you have a minute maybe you could give it a run.

MediaWiki 1.27.0-alpha (86c08b2)

We can't use 1.27 since all our integration tests are broken after 2016-01-24 (caused by some MW-core change) which also includes #1295.

@cicalese
Copy link
Contributor

@cicalese cicalese commented Feb 17, 2016

OK, I will back off to an earlier MW version.

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 17, 2016

... "Modification date" changes from text to a link:

I've never seen this behaviour and our sandbox [0] also doesn't indicate that such phenomenon (aside from the current PR) is observable.

[0] http://sandbox.semantic-mediawiki.org/wiki/Main_Page

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 17, 2016

But, when I refresh the page, it disappears:

That shouldn't happen because _DYTLE (Display title of) is set by (or better in) the same hook NewRevisionFromEditComplete as _MDAT (Modification date) which is only executed after an edit.

@kghbln
Copy link
Member

@kghbln kghbln commented Feb 17, 2016

In case you have a minute maybe you could give it a run.

I currently cannot go 1.27 since this version now requires PHP 5.5.9+ or HHVM 3.1+ which is probably the reason for the tests to fail. I will see to it that the sandbox server (#1186) gets an upgrade and go 1.27-alpha thereafter.

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 17, 2016

now requires PHP 5.5.9+ or HHVM 3.1+ which is probably the reason for the tests to fail.

The test failures have nothing to do with the PHP version, they pass on MW 1.26/PHP 5.6.

@kghbln
Copy link
Member

@kghbln kghbln commented Feb 17, 2016

The test failures have nothing to do with the PHP version, they pass on MW 1.26/PHP 5.6.

Ouch, so this is worse than I had hoped. Sometimes dreams die a sudden death I suppose. :(

@cicalese
Copy link
Contributor

@cicalese cicalese commented Feb 17, 2016

I have now tested under:

MediaWiki 1.26.2 (c2d388d)
PHP 5.5.32 (apache2handler)
MySQL 5.5.48

The patch is working better in that environment. When I create a page, the "Display title of" property is getting set. However, when I run the refreshLinks/runJobs maintenance scripts, the property goes away. I am suspicious that no jobs are being put on the job queue by refreshLinks. I also tried running the SMW rebuildData script, but it did not help. If I edit the page and re-save it, the property comes back.

As an additional test, I added in the JavaScript changes from my earlier pull request and uncommented the line in the serialization code. With those modifications, the datatables result format beautifully renders the displaytitle as the link text. This is HUGE progress, and I do hope you will consider incorporating these additions to SMW.

Thank you.

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 17, 2016

I have now tested under:

Thanks for testing.

I also tried running the SMW rebuildData script, but it did not help. If I edit the page and re-save it, the property comes back.

Since we rely on ...->getProperty( 'displaytitle' ) I moved from the NewRevisionFromEditComplete to the ParserAfterTidy hook which allows to fetch the 'displaytitle' on a regular purge or update instead of only on an edit which should also be sufficient for the rebuildData script.

@cicalese
Copy link
Contributor

@cicalese cicalese commented Feb 19, 2016

@mwjames, I don't see a new commit to test that the refreshLinks bug is fixed. Is there anything else I can do to help move this forward? Have you had a chance to assess any performance impact (hopefully minimal) to uncommenting the display title query in the serialization code? If you are satisfied that it does not have a significant impact, I would like to propose adding the JavaScript modifications from my earlier patch to this or a following patch. Thank you.

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 24, 2016

I don't see a new commit to test that the refreshLinks bug is fixed. Is there anything else I can do to help move this forward?

There you go.

Have you had a chance to assess any performance impact (hopefully minimal) to uncommenting the display title query in the serialization code?

Indeed I did (always a daunting task) and after some back and forth where the InMemoryCache did cache repeated requests for the session I wasn't satisfied that it would still engage in store requests. Since I can not assume that every user has enabled the CachedValueLookupStore, I decided to introduce CachedPropertyValuesPrefetcher so that preselected property lookups (mostly for high frequent usages) can divert to a persistent cache (using redis) without compromising the overall data integrity where each time a subject is changed the cache for the entity is evicted as well.

The CachedPropertyValuesPrefetcher also solves my concerns over repeated requests from the API (each subject/subobject are to be checked at least once during a request to see whether a display title was set or not).

On a different note, if a subobject does not assign a Display title of property as part of its declaration, it will inherit the display title of its parent (the entity that embeds the subobject).

Now that subobjects are considered as well, I've decided to place values from _DTITLE in an extra fpt table therefore running update.php or setupStore.php is required.

SMW_DV_WPV_DTITLE has been added as feature flag to $GLOBALS['smwgDVFeatures'] in order to disable the caption usage for wikipage dataValues and restore the current behaviour by removing the entry from the setting.

SMW_DV_PROV_DTITLE has been added as option but is disabled by default (see the settings comment).

impact (hopefully minimal) to uncommenting the display title query in the serialization code?

I leave the code commented in the serializer so that is unrelated to the current PR (and can be enabled by your PR together with bumping the version number).

@cicalese
Copy link
Contributor

@cicalese cicalese commented Feb 29, 2016

I just this very moment discovered the same thing and was planning to write to @kghbln about it.

@kghbln
Copy link
Member

@kghbln kghbln commented Feb 29, 2016

Done now. I got a bit distracted by my German IBAN regex. All cool now.

@cicalese
Copy link
Contributor

@cicalese cicalese commented Feb 29, 2016

Thank you, @kghbln!

All, you can now see a working test of this functionality at [0].

The first tab, table, is a baseline which was already working before this patch.

@Fannon, does the example in the filtered tab address your original concern? This fix did not require #1423 since the filtered result format does not use JavaScript to render the filters. But, maybe there was an additional issue with the filtered result format.

When the sandbox is updated to include the recently merged #1423, the display titles of the pages on the datatables tab will render correctly.

@mwjames, you can also see the other unrelated bug in action on the datatables tab by refreshing the table with the button in the top right. All of the links become red. I will add a separate bug report for that linking to the example in the sandbox.

[0] http://sandbox.semantic-mediawiki.org/wiki/Display_Title_Test

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Feb 29, 2016

can also see the other unrelated bug in action on the datatables tab by refreshing the table with the button in the top right.

I think this worked in previous releases (not sure though) but since the [0] JSON format change the following "...Modules that were previously outputting raw booleans in JSON may now output those properties using the convention that has always been the standard (for version 1): empty-string for true and absent key for false...." may pushed this over the cliff.

[0] https://www.mediawiki.org/wiki/API:JSON_version_2

@cicalese
Copy link
Contributor

@cicalese cicalese commented Feb 29, 2016

I just opened #1428 for that unrelated bug. Hopefully with the diagnosis, the fix will be straightforward.

mwjames added a commit that referenced this issue Mar 8, 2016
Strip tags from displayTitle, refs #1410
mwjames added a commit that referenced this issue Apr 28, 2016
`Display title of` to augment sortkey, refs #1410
mwjames added a commit that referenced this issue May 26, 2016
Add DataValue::getPreferredCaption, refs #1410
mwjames added a commit that referenced this issue Aug 24, 2016
Special:Browse to show redirect by source without displaytitle, refs #1410
@kriegfrj
Copy link

@kriegfrj kriegfrj commented Aug 29, 2016

How does this feature addition tie in with the Semantic Title extension (https://www.mediawiki.org/wiki/Extension:Semantic_Title)? Does it make it redundant or partially redundant, or is it complementary?

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Sep 1, 2016

How does this feature addition tie in with the Semantic Title extension (https://www.mediawiki.org/wiki/Extension:Semantic_Title)? Does it make it redundant or partially redundant, or is it complementary?

I don't know but maybe @cicalese @Fannon can answer this question otherwise I'd suggest you make an inquiry on the extension page.

If someone has an answer to this question it may prove helpful to extend [0] as orientation for other users.

[0] https://www.semantic-mediawiki.org/wiki/Help:Special_property_Display_title_of

@cicalese
Copy link
Contributor

@cicalese cicalese commented Sep 1, 2016

This feature,(replacing the page title with the display title, if present, in links to a page in query results and adding a Semantic MediaWiki property holding the display title) is complementary to the functionality provided by the SemanticTitle extension.

SemanticTitle performs several important functions in addition to this feature:

  • replaces the page title with the display title, if present, in wikitext links to a page as long as alternate link text is not provided (both links from other pages as well as self links, the latter of which appear as bold text on the page)
  • adds a parser function to return the display title of a page (works in the absence of Semantic MediaWiki, which provides the same data through a query)
  • optionally adds a subtitle containing the page title to pages with a display title

In addition, for legacy reasons, SemanticTitle supports setting the display title of a page from the value of a Semantic MediaWiki property or Cargo field. This was rendered unnecessary in version 3.0 of SemanticTitle, which retrieves the display title from the same mechanism used by the MediaWiki DISPLAYTITLE magic word (the displaytitle value in the page_props database table). Setting the display title using the DISPLAYTITLE magic word is both more efficient and behaves better with respect to caching than using the legacy mechanisms, which will be removed in a future version of SemanticTitle.

SemanticTitle no longer requires Semantic MediaWiki - although they are compatible and, as described above, complementary. It is possible that SemanticTitle will be renamed DisplayTitle in the future, to more closely align it with its relationship to DISPLAYTITLE and to make it clear that it does not require Semantic MediaWiki. This would be done in conjunction with removing the legacy mechanisms described above. It is also possible that we might try to have the three SemanticTitle features listed above accepted as optional capabilities in MediaWiki core.

@kriegfrj
Copy link

@kriegfrj kriegfrj commented Sep 2, 2016

Thanks for that summary, @cicalese. I managed to find out that they were compatible - I simply changed the SemanticTitle configuration to use the Display title of built-in property rather than my own, and then the #semantic-title parser function worked. I agree that this parser function it would make sense for it (or an equivalent) to be somewhere in the MW core - it is often handy to find out what the display title is of another page.

Interesting comment about using the DISPLAYTITLE tag directly rather than via a property as being more efficient - is this true also of the Display title of property (ie, is it more efficient to use DISPLAYTITLE than to use Display title of property)?

@mwjames
Copy link
Contributor Author

@mwjames mwjames commented Sep 2, 2016

Interesting comment about using the DISPLAYTITLE tag directly rather than via a property as being more efficient - is this true also of the Display title of property (ie, is it more efficient to use DISPLAYTITLE than to use Display title of property)?

To clarify further on this issue and as noted in [0], this feature is about using DISPLAYTITLE as method to influence the display of a page entity. The Display title of property is to indicate to other SMW related processes to use its value as preferred caption. Using Display title of itself may make sense in some situations [1] but best practice is to rely on DISPLAYTITLE to modify the display characteristics as we do in a similar case with DEFAULTSORT to alter the sort attribute.

[0] https://www.semantic-mediawiki.org/wiki/Display_title
[1] #1410 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants