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

Remove unused fieldsets from Category List #10055

Closed
wants to merge 1 commit into from

Conversation

smz
Copy link
Contributor

@smz smz commented Apr 22, 2016

Summary of Changes

In "Category List" we have (since Joomla 3.0 if I'm not mistaken) two fieldsets and related "tabs" which I think are of no usage: "Options" (which is for setting articles options) and "Integration" (for setting RSS feeds related options), but in "Category List" we just build up a list of articles with just links to them: no article has ever been displayed by that view and hence all related options are totally ineffective.

Testing Instructions

Not much to test... everything should work "business as usual"...

@brianteeman
Copy link
Contributor

Integration tab for rss feeds is used - you can see that by viewing the source with feeds on and feeds off

Options is used - you can see this by setting the first option - show title to hide


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10055.

@richard67
Copy link
Member

@smz Sure you changed the righ file? I would expect the categoy list to be "components/com_content/views/categories/tmpl/default.xml" and not "components/com_content/views/category/tmpl/default.xml", which is the one you changed. The latter one is the one where you also can find the blog views, so there the article-related options should be kept, or am I wrong?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10055.

@brianteeman
Copy link
Contributor

Closed as this is simply wrong


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10055.

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

@richard67 the xml for the "Category Blog is "components/com_content/views/category/tmpl/blog.xml" (not "default.xml")

@richard67
Copy link
Member

And what is "components/com_content/views/categories/" good for then?

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

it is for the "List All Categories" view/menu item

@brianteeman
Copy link
Contributor

It doesnt matter what file it is. As I stated before the options are in use
as can be clearly seen if you change the values

On 22 April 2016 at 23:21, Richard Fath notifications@github.com wrote:

And what is "components/com_content/views/categories/" good for then?


You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#10055 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

No Brian, they are not: maybe just the title thing (I'm checking right now), but not all the others... think it over...

@richard67
Copy link
Member

@smz Ah I see, thanks for the info.

@brianteeman
Copy link
Contributor

I cant help it if you are wrong but i can stop you having bad code accepted

On 22 April 2016 at 23:25, Richard Fath notifications@github.com wrote:

@smz https://github.com/smz Ah I see, thanks for the info.


You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#10055 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

Sorry, Brian, you're wrong: the title is controlled by the "Category Title" option in the "Category" tab, not in the "Options" tab. Please test...

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

And as far as regards the RSS feed there is not a single line of PHP code referencing it in the view. Please review...

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 2016

The RSS stuff is coming from the parent JViewCategory class: https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/view/category.php#L302

Don't just look at the single class and assume it's unused. You also have to look at parent classes in some cases.

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

The RSS stuff is coming from the parent JViewCategory class: https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/view/category.php#L302

... oops!

Your opinion about article's option @mbabker ?

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 2016

I don't know. Without explicitly tracing every single option you can't say what's used or not used where. There's a part of me that doubts though that an entire tab is totally ignored.

@brianteeman
Copy link
Contributor

As you dont seem to believe me I made a video of core joomla with testing
sample data showing that both the integrations tab and the options tab are
being used.

https://www.dropbox.com/s/j0aooskw0g9ru4z/10055.mp4?dl=0

On 22 April 2016 at 23:37, Michael Babker notifications@github.com wrote:

The RSS stuff is coming from the parent JViewCategory class:
https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/view/category.php#L302

Don't just look at the single class and assume it's unused. You also have
to look at parent classes in some cases.


You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#10055 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman
Copy link
Contributor

The video is the proof - I didnt just close the PR because I dont like you.
I closed the PR because you were completely wrong

On 22 April 2016 at 23:42, Michael Babker notifications@github.com wrote:

I don't know. Without explicitly tracing every single option you can't say
what's used or not used where. There's a part of me that doubts though that
an entire tab is totally ignored.


You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#10055 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

Apart the RSS thing (with which I've to admit I'm not familiar, and which I'm reverting) my rationale is the if that there is not a single point in the view's template where those option are referenced (and we are talking about options that should have a visible effect on the generated view), then the options are "unused".

But I might be wrong: I see while I'm writing the @brianteeman has prepared a video to look at... give me the time to watch it, please...

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 2016

Ya know what, I change my mind. I have no freakin' clue anymore what's used where or not used. A search on the show_title param (https://github.com/joomla/joomla-cms/search?utf8=%E2%9C%93&q=show_title&type=Code) honestly leaves me clueless whether it is actually used by core in the default layout (or its sublayouts) of com_content's category view.

Even if it isn't, if a template's implemented the param though, it's a B/C break.

And people wonder why I hate how many options are in the Joomla UI...

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

Even if it isn't, if a template's implemented the param though, it's a B/C break.

I don't think so as in case of an override also the xml should be overridden (or am I wrong ?)

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 2016

You can create a layout override without creating a XML override.

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

@brianteeman

I've watched your screen capture and I totally agree as far as regards the RSS feed thing. I was wrong on that.

As far as regards the article titles appearing/disappearing in the single article view... I don't understand... is it because the "Category List" is the only menu item "controlling" the the article view?

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 2016

Another thought dawned on me. The params may not even apply directly to the category view but rather articles on the article view which have inherited the menu item params for its parent category view.

Translation. I ain't got a clue, and I honestly don't care enough to find out. There are too many possible combinations to test effectively.

@richard67
Copy link
Member

is it because the "Category List" is the only menu item "controlling" the the article view?

Yes, I think it depends on if you have a menu item for the single article or not.

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

Translation. I ain't got a clue, and I honestly don't care enough to find out. There are too many possible combinations to test effectively.

Yes... I guess it all comes from the "somehow unpredictable" or at least "difficultly predictable" way the router associates "things" and what comes in control as far as regards options...

@brianteeman
Copy link
Contributor

No it is perfectly predictable and working exactly how it is supposed to be
working

On 23 April 2016 at 00:01, Sergio Manzi notifications@github.com wrote:

Translation. I ain't got a clue, and I honestly don't care enough to find
out. There are too many possible combinations to test effectively.

Yes... I guess it all comes from the "somehow unpredictable" or at
least "difficultly predictable" way the router associates "things" and
what comes in control as far as regards options...


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10055 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

No it is perfectly predictable and working exactly how it is supposed to be working

Have a look at #9717 ...

@richard67
Copy link
Member

Anyway ... even if this PR was wrong, I learned something new from it.

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

Anyway ... even if this PR was wrong, I learned something new from it.

me too...

@richard67
Copy link
Member

Question is if it is worth to keep that all in mind when things will soon change completely with 4.0 👅

@richard67
Copy link
Member

was joking

@smz
Copy link
Contributor Author

smz commented Apr 22, 2016

soon? 😕

@richard67
Copy link
Member

months ago if looking at old project schedules 😄

@smz smz deleted the SMZ-CategoryListOptionsCleanup branch April 22, 2016 23:08
@Bakual
Copy link
Contributor

Bakual commented Apr 23, 2016

It's really simple. The options aren't used in the cageories list, but they are used once you navigate from the categories list down to a single category or article view. Then the params from the menu take precedence over the global component params and you can eg hide the author in that specific menu item.
If the category or article item has own options set, then it gets a bit more complex, but that's not the topic here 😄

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

Successfully merging this pull request may close these issues.

None yet

6 participants