Unify generic object toolbar selection with the other toolbars.

[martinpovolny]

Use #621 to unify how toolbars are selected for Generic Objects.

Fixes part of #1238

[miq-bot]

Checked commits martinpovolny/manageiq-ui-classic@f41d7b8~...3cdeee3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 2 offenses detected

app/helpers/application_helper/toolbar/generic_object_definition_center.rb

• ❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
• ❗ - Line 46, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.

[eclarizio]

Everything seems fine for this PR. However, I still don't like how the x_history toolbar and the GTL toolbar just kind of magically happen within the ToolbarChooser. (Or don't happen in the case of GTL)

Is the goal that we specify all toolbars within the controller in a certain order or something like this?:

history_tb :x_history
center_tb :generic_object_definition
view_tb :blank_view

And then if we needed any custom ones, it would be similar?:

custom_tb :my_custom_tb

I think that's similar to what I was originally attempting to do when I made the GenericObjectHelper, and it seems a lot cleaner to me than having to maintain a huge if/else block in the ToolbarChooser. If that's the way we want to go going forward, then I'm all for it 👍 .

[eclarizio]

I think we can get rid of generic_object_definition, there is nowhere that currently uses that layout, as far as I could find, and it probably should've been removed back when I originally wrote it.

[martinpovolny]

I am ok to remove it. I did not find why that value was there and did not dare removing it.

[martinpovolny]

However, I still don't like how the x_history toolbar and the GTL toolbar just kind of magically happen within the ToolbarChooser. (Or don't happen in the case of GTL)

Neither do I but it's not the topic of this PR to resolve that. I prefer to limit the scope of PR.

Is the goal that we specify all toolbars within the controller in a certain order or something like this?:

history_tb :x_history
center_tb :generic_object_definition
view_tb :blank_view

I don't think that is the solution. I think that the logic behind the display of the view toolbar/toolbars should be elsewhere, it should be part of the layout of the particular page not the controller. I have not yet figured that out.

In case of view toolbar for GTL then next step is to couple that closer with the GTL component and to process that on client side w/o server roundtrip.

Similar applies to the history toolbars: At this point I am not sure what are the criteria for that toolbar to be displayed. It's surely not (just) the controller. I think we need some UX work to properly understand when that should be displayed and when not. And that is also out of scope here.

[eclarizio]

Neither do I but it's not the topic of this PR to resolve that. I prefer to limit the scope of PR

I agree, I was just trying to generate a discussion around what you were thinking about going forward in terms of how to handle these types of things.

So, yeah, like I said before, this PR is fine by me 👍