Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The order of the views displays get messed up in the config entity.
Proposed resolution
Sort views display always by display name in the config entity to generate minimal diffs in CMI.
Remaining tasks
None
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#36 | 2350821.36.patch | 4.28 KB | alexpott |
#36 | 34-36-interdiff.txt | 899 bytes | alexpott |
#34 | 2350821.34.patch | 3.4 KB | alexpott |
#34 | 25-34-interdiff.txt | 6.34 KB | alexpott |
#25 | interdiff.txt | 2.48 KB | dawehner |
Comments
Comment #1
webflo CreditAttribution: webflo commentedComment #2
webflo CreditAttribution: webflo commentedComment #4
dawehnerThe failures are a little bit more tricky than expected.
Comment #6
webflo CreditAttribution: webflo commentedReroll
Comment #7
webflo CreditAttribution: webflo commentedComment #9
dawehnerThere we go.
Comment #10
damiankloip CreditAttribution: damiankloip commentedThis I am ok with. The position is more a UI thing anyway.
Oops
Isn't a #weight property already added in getDisplaytabs()? Also, the hardcoding of page, block seems a bit meh.
Comment #11
dawehnerWell, this is sadly a bit tricky to be honest. We do sort the displays with the following code:
which means that the order of the tabs is (as you would expect) caused by the priority managed in the UI.
Meh I don't have a better suggestion for that at the moment but hardcoding here feels better than the clusterfuck of moving displays around all the time.
Where is the problem ;)
Comment #12
Gábor HojtsyBased on discussion on last week's CMI meeting, I opened #2361539: Config export key order is not predictable for sequences, add orderby property to config schema .
Comment #13
alexpottUnfortunately #2361539: Config export key order is not predictable for sequences, add orderby property to config schema probably won't help here since displays is a sequence. Unless decide to have a new rule that all sequences are key sorted. Which would not be so silly imho since what we could then say is mappings are sorting according to schema, associated arrays (sequences) are key sorted and indexed arrays (sequences too) are saved in the order provided.
The one issue with this idea is code like this where we want the default to always come first.
Comment #14
alexpottSetting back to needs work because we totally needs tests for this type of change cause it is so easy to break.
Comment #15
Gábor Hojtsy#2361539: Config export key order is not predictable for sequences, add orderby property to config schema would only help if we decide to introduce something like an "order_by_value_of_key: id" or something along those lines, where we would provide the name of key to take the value to order the sequences by. That may or may not be a good idea or in scope there. I would not postpone this issue on that.
Comment #16
vijaycs85Adding views UI test.
Comment #19
webflo CreditAttribution: webflo commentedWe should save the view after we added the block display. Otherwise the changes are not persistent right?
I could not gork the uksort callback. I think is does not work in all cases. I tried to refactored it into something easier.
@alexpott
$this->set('display', array('default' => $display['default']) + $display);
This line should prioritize default.
Comment #21
vijaycs85Thanks @webflo. looks good. here is the test-only patch for #19 to prove it fails.
/me saved the patch as 2350821-view-display-order-19-test-only.php. May be bed time :)
Comment #23
dawehnerThe other implementation indeed looks a bit nicer.
Huch, I am confused, don't we hide the master display by default, so that there is also no 'Master' on that page? The text feels a bit misleading.
Should we also explicit check the order in the UI instead of just the config?
Comment #24
alexpottThis is a bug as can be seen from the patch attached to #2316909-17: Revisit all built-in test/default views configuration in core - the orders of things shouldn't change just because we save them. Also this kind of blocks progress on that issue so bumping priority.
Comment #25
dawehnerComment #26
alexpottHow come we're not sorting by weight in the UI?
Comment #27
alexpottI've just postponed #2316909: Revisit all built-in test/default views configuration in core on this - so this is now critical
Comment #28
dawehner@alexpott
Well, you know, people came up with that behaviour in order to improve UX in d7.
The page display is considered as more important than a block display, especially because on
admin/structure/views/add
you see the same kind of order.
Comment #29
alexpottThis is unnecessary since the default display is only shown when it is the only display.
This functionality appears untested since we add page_1 and block_1 in the test. If a view did have view displays with these ID then it would break the ability to reorder displays in the UI.
Basically I don't see the point of
ViewFormBase::sortDisplayTabs()
or theDisplayOrderTest
test since views display re-ordering is tested inDisplayTest::testReorderDisplay()
Comment #30
alexpottSo #29.1 is wrong since this is configurable. but there is already code preventing you from re-ordering that so I still stand behind my statement that I don't see the need for the test or the code.
Comment #31
alexpottComment #32
olli CreditAttribution: olli commentedWhat is the problem with storing the displays in the same order as they appear in UI?
Comment #33
alexpott@olli because then when all you do is re-order them you get a massive diff when all that has changed is the weight.
Comment #34
alexpottSo manual testing has proved that the master display always comes first in the UI and users can re-order to whatever they like and this is tested. So this patch should just concentrate on the order that displays are saved.
Patch attached does that.
Comment #36
alexpottOk we need to sort the tabs by weight to choose the correct one when no display is set.
Comment #37
dawehnerThank you @alexpott!!
Alright, Page still appears before block +1
Comment #38
catchCommitted/pushed to 8.0.x, thanks!
Comment #40
olli CreditAttribution: olli commented#33: Ok, thanks!
I think it might be a good idea to add a method somewhere which you can use to get a list of view displays ordered by position (the same order as display tabs in UI). That could be used in views ui for "attach to", "link display" and in view area plugin.