[FIX] borders: preserve side borders when combining External and All #7471
Conversation
|
|
|
I feel like applying the opposite border strategy (left <>right, top<>bottom) should actually be done in https://github.com/odoo/o-spreadsheet/pull/7471/files#diff-775eae5d11f82632d3abbe31764630b6ee83acd820f4251d4e2a10c2c1e4a122R345 ( I still fail to understand why this only impacts the import and is not triggered during the actual use but it might come from the fact that we never functionally have the opportunity to create a border with external borders only applying to top,left,bottom and nothing on right. it might be interesting to see if such a command can be dispatched, and if so, it should be tested as well for good measure and the test might be more explicit than an import/export (at first glance, i think this can be achieved by dispatching |
dhrp-odoo
force-pushed
the
19.0-fix-border-lost-after-refresh-dhrp
branch
from
2ec7c34 to
8c55974
Compare
|
@rrahir right, I just inverted the mapping in Added a test to cover this via commands (and removed the import test, since it felt almost like a duplicate) Thanks for taking the time to review this PR 🙂 |
rrahir
left a comment
rrahir
left a comment
There was a problem hiding this comment.
Still trying to understand if the previous version worked somehow. the adjacent computations are not very clear to me/. I would like to have more explanations on each step because it's hard to understand if the modifications are done on the existing border or the one we want to add ...
| }); | ||
| }); | ||
|
|
||
| test("Preserves side borders when combining external and all via command", () => { |
There was a problem hiding this comment.
I think we should be very thorough here because it also affects top/bottom similarly., add some dispatch with "empty" border adjacents to C2:C4 and check that every single cell is still protected.
tests/borders/border_plugin.test.ts
Outdated
| left: defaultBorder, | ||
| right: defaultBorder, | ||
| }); | ||
| setZoneBorders(model, { position: "hv" }, ["C2:C4"]); |
There was a problem hiding this comment.
| setZoneBorders(model, { position: "hv" }, ["C2:C4"]); | |
| setZoneBorders(model, { position: "all" }, ["C2:C4"]); |
is equivalent to both commands you dispatch above, isn't it?
| right: defaultBorder, | ||
| }); | ||
|
|
||
| expect(getBorder(model, "C2")).toEqual({ |
There was a problem hiding this comment.
you should check every single cell and verify that they were not altered ;-)
src/plugins/core/borders.ts
Outdated
| ) { | ||
| const borders: ZoneBorder[] = []; | ||
| const plannedBorder = newBorder ? { zone, style: newBorder } : undefined; | ||
| const sideToClear = { |
There was a problem hiding this comment.
this corresponds to the side to clear in the existing borders in case of adjacency (maybe the right term is contiguity, dunno) and no intersection
| const sideToClear = { | |
| const existingBorderSideToClear = { |
maybe?
When applying an External border on a range and then applying All borders on an inner/overlapping range, the side borders (left/right) of the inner cells were lost after re-opening the spreadsheet. The exported border data was correct, but the BordersPlugin recomputation logic in addBorder incorrectly cleared adjacent borders on import/update. The adjacency check (adjacent(existingBorder.zone, zone)) reported the side of the existing zone, but we were deciding whether to clear it based on the same side key in the new border, instead of the opposite side (shared edge) on the new zone. As a result, importing the combination of: C2:C4 with All borders B2:B4 with left/top/bottom D2:D4 with right/top/bottom ended up clearing the left and right borders of C2:C4. This commit adjusts the adjacent clearing logic to: Map the adjacent side of the existing zone to the opposite side on the new zone (i.e. left → right, right → left, top → bottom, bottom → top). Only clear the existing side if the corresponding opposite side is actually being written on the new border. Task: 5270171
dhrp-odoo
force-pushed
the
19.0-fix-border-lost-after-refresh-dhrp
branch
from
8c55974 to
4e27d00
Compare
|
@rrahir Indeed, the implementation is a bit confusing, especially because the bug only shows up on the opposite side. I’ve added a comment to clarify the logic and extended the test to also cover the top/bottom cases (took a bit of time to reproduce, it wasn’t obvious at first glance 😃). |
3 participants
Description:
When applying an External border on a range and then applying All borders on an inner/overlapping range, the side borders (left/right) of the inner cells were lost after re-opening the spreadsheet.
The exported border data was correct, but the BordersPlugin recomputation logic in addBorder incorrectly cleared adjacent borders on import/update. The adjacency check (adjacent(existingBorder.zone, zone)) reported the side of the existing zone, but we were deciding whether to clear it based on the same side key in the new border, instead of the opposite side (shared edge) on the new zone. As a result, importing the combination of:
C2:C4 with All borders
B2:B4 with left/top/bottom
D2:D4 with right/top/bottom
ended up clearing the left and right borders of C2:C4.
This PR adjusts the adjacent clearing logic to:
Map the adjacent side of the existing zone to the opposite side on the new zone.
Only clear the existing side if the corresponding opposite side is actually being written on the new border.
Task: 5270171
review checklist