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

Listwidget format: No longer working #449

Closed
m-art-in opened this issue Nov 22, 2018 · 14 comments
Closed

Listwidget format: No longer working #449

m-art-in opened this issue Nov 22, 2018 · 14 comments
Labels

Comments

@m-art-in
Copy link

Setup

  • MW version: 1.31.1
  • DB (MySQL etc.): MariaDB 10.2.15
  • PHP version: 7.2.5
  • SMW version: 3.0.0
  • SRF version: 3.0.0

Issue

Since the update to lastes SMW and SRF the listwidget is broken. Even the official demo pages are not working anymore:

https://www.semantic-mediawiki.org/wiki/Demo:Listwidget_alphabetic_list

@kghbln kghbln added the bug label Nov 22, 2018
@kghbln
Copy link
Member

kghbln commented Nov 22, 2018

Thanks a lot for reporting. Yeah, this is really bad since it is a very useful and often used format.

@kghbln kghbln changed the title Listwidget format is broken Listwidget format: No longer working Dec 15, 2018
@kghbln
Copy link
Member

kghbln commented Jan 13, 2019

@mwjames It will be utterly great if you had time for a sprint here. This is probably the collateral damage with the most negative impact to wikis out there.

@mwjames
Copy link
Contributor

mwjames commented Jan 13, 2019

I believe it relates to the changes of the ul/ol printer which this one depends on, I'm not sure when I'll be able to have a look at it but maybe adding something like:

 	 * @see SMWResultPrinter::getResultText
 	 *
 	 * @param SMWQueryResult $res
 	 * @param array $params
@@ -37,11 +53,12 @@ class SRFListWidget extends ListResultPrinter {
 		// Initialize
 		static $statNr = 0;
 		//$this->isHTML = true;
 
 		// Set output type for the parent
-		$this->mFormat = $this->params['listtype'] == 'ordered' || $this->params['listtype'] == 'ol' ? 'ol' : 'ul';
+		$this->params['format'] = $this->params['listtype'] == 'ordered' || $this->params['listtype'] == 'ol' ? 'plain-ol' : 'plain-ul';
+		$this->params['sep'] = '';

...

 		// Wrap results
 		return Html::rawElement(
 			'div',
 			[
 				'class' => 'srf-listwidget ' . htmlspecialchars( $this->params['class'] ),
-				'data-listtype' => $this->mFormat,
+				'data-listtype' => $this->params['listtype'] == 'ordered' || $this->params['listtype'] == 'ol' ? 'ol' : 'ul',
 				'data-widget' => $this->params['widget'],
 				'data-pageitems' => $this->params['pageitems'],
 			],
 			$processing . $result
 		);

The problem is that we need something like 'plain-ol' : 'plain-ul'; where ul/ol is returned as plain ul/ol without html.

@kghbln
Copy link
Member

kghbln commented Jan 13, 2019

Thanks a ton for the swift response and your suggestion! I will give this a shot and do a pull request if the suggestion works. Touching wood since this is rather a show stopper.

@mwjames
Copy link
Contributor

mwjames commented Jan 13, 2019

I will give this a shot and do a pull request if the suggestion works. Touching wood since this is rather a show stopper.

It won't unless 'plain-ol' / 'plain-ul' is created which currently doesn't exist in smw-core.

@kghbln
Copy link
Member

kghbln commented Jan 13, 2019

It won't unless 'plain-ol' / 'plain-ul' is created which currently doesn't exist in smw-core.

Would it be sufficient to add them via the configuration parameter for result format aliases?

@kghbln
Copy link
Member

kghbln commented Jan 13, 2019

Would it be sufficient to add them via the configuration parameter for result format aliases?

Probably not since we still need "ol" and "ul" to strip html like currently only "plainlist" does. Ouch, this is bad.

@mwjames
Copy link
Contributor

mwjames commented Jan 13, 2019

Probably not since we still need "ol" and "ul" to strip html like currently only "plainlist" does. Ouch, this is bad.

The JS used to generate these lists expects simple ul/ol (aka plain) elements and gets confused by any other HTML that surrounds them hence the need for the plain-... output.

@kghbln
Copy link
Member

kghbln commented Jan 13, 2019

I will create a feature request at SMW for the plain formats. Anyways we are looking at SWM/SRF 3.1.0 minimum which is a crucial information to work with for wiki update planning.

@s7eph4n
Copy link
Contributor

s7eph4n commented Jan 18, 2019

As mentioned over on SMW #3623 it might be sufficient to set $this->params['format'] to "ol" or "ul", i.e. no "plain-ol" or "plain-ul" necessary.

 	 * @see SMWResultPrinter::getResultText
 	 *
 	 * @param SMWQueryResult $res
 	 * @param array $params
@@ -37,11 +53,11 @@ class SRFListWidget extends ListResultPrinter {
 		// Initialize
 		static $statNr = 0;
 		//$this->isHTML = true;
 
 		// Set output type for the parent
-		$this->mFormat = $this->params['listtype'] == 'ordered' || $this->params['listtype'] == 'ol' ? 'ol' : 'ul';
+		$this->params['format'] = $this->params['listtype'] == 'ordered' || $this->params['listtype'] == 'ol' ? 'ol' : 'ul';

...

 		// Wrap results
 		return Html::rawElement(
 			'div',
 			[
 				'class' => 'srf-listwidget ' . htmlspecialchars( $this->params['class'] ),
-				'data-listtype' => $this->mFormat,
+				'data-listtype' => $this->params['format'],
 				'data-widget' => $this->params['widget'],
 				'data-pageitems' => $this->params['pageitems'],
 			],
 			$processing . $result
 		);

Of course, this still leaves the ListWidgetPrinter relying on an internal interface ($this->params) and might break at some point. So the better solution might be not to inherit from ListResultPrinter at all. Composition over inheritance and all that.

@kghbln
Copy link
Member

kghbln commented Jan 18, 2019

@s7eph4n Thanks for your comment. I will try the proposed code changes this weekend and see what happens.

I guess doing some kind of refactoring is another story for a second step.

@kghbln
Copy link
Member

kghbln commented Jan 19, 2019

I just did some field testing and the listwidget is working great again. However one now needs to add |sep= to avoid separators unfluffing the result. However if this is what it takes to move forward I am happy to do so. :)

@m-art-in
Copy link
Author

Thank you very much for your work and commitment!
When do you expect version 3.1 ( or 3.0.1) to be released?

@kghbln
Copy link
Member

kghbln commented Feb 13, 2019

Thank you very much for your work and commitment! When do you expect version 3.1 ( or 3.0.1) to be released?

I wanted to do a 3.0.1 a fortnight ago but other issues and low availability got into my way. Since this is the major blocker for me migrating all the wikis I control to SMW 3.0 etc I hope to be able to fix the test this Sunday. If @s7eph4n could move in the separator tweaks in this will be utterly cool, too. To cut a long story short. I cannot take to long, at least I hope.

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

No branches or pull requests

4 participants