refactor(ui): a rule between two things belongs to nobody - #82
Merged
Conversation
Twelve of them existed: nine horizontal under three different names — `divider` eight times, `horizontal_rule` once — and three vertical. Six of the nine were identical character for character. They could not simply be shared, and the two reasons are worth naming because they are why this is a module rather than a deletion. They disagreed about their argument. Seven took `&Palette` and five took an `iced::Color` the caller had already pulled out of one, which leaves each call site deciding what colour a rule is — the decision that should not be made twelve times. The answer is that a rule is always `palette.border`: taking the colour away from the call site is most of the point, and a container that is any other colour is not a rule but a band, which is a different thing with a different job. They also disagreed about their message type, some returning `Element<Message>` and some `Element<RepoMessage>`. That reads like the harder obstacle and is the easier one: a rule emits nothing, so the message type is free and these are generic over it. Their thickness is `metrics::HAIR` rather than a `1.0` written out three ways — `Length::Fixed(1.0)` on the space, `1` on the space, and `Length::Fixed(1.0)` on the container all appeared, for the same line. The guard reads the directory rather than a list, so a widget added tomorrow is covered without anyone remembering to add it, and it has a second test that it reads anything at all — a guard that quietly walked an empty tree would pass forever, which is the failure mode a source-scanning test actually has. This settles the argument convention for the shared helpers only. The crate-wide split between `&Palette` and `Palette` by value is still there, in the widgets' own signatures, and is not what stood in the way here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
C1. Twelve rules existed: nine horizontal under three different names —
dividereight times,horizontal_ruleonce — and three vertical. Six of the nine were identical character for character.Why this needed a module and not a delete key
They disagreed about two things, and only one of them was a real obstacle.
Their argument. Seven took
&Palette; five took aniced::Colorthe caller had already pulled out of one. The colour form leaves every call site deciding what colour a rule is — precisely the decision that should not be made twelve times. So the shared one takes&Paletteand a rule is alwayspalette.border. Taking the colour away from the call site is most of the point: a container that is some other colour is not a rule, it is a band, and a band is a different thing with a different job.Their message type. Some returned
Element<Message>, someElement<RepoMessage>. This reads like the harder problem and is the easier one — a rule emits nothing, so the message type is free and the helpers are generic over it.And their thickness was written three ways
Length::Fixed(1.0)on the space,1on the space, andLength::Fixed(1.0)on the container — all for the same line. It ismetrics::HAIRnow, which is what T1 was for.The guard, and the guard's guard
The test reads the directory, not a list, so a widget added tomorrow is covered without anyone remembering to add it.
It has a second test that it reads anything at all. That is not padding: a source-scanning guard's real failure mode is walking an empty tree and passing forever. Sabotaging the directory list to
[]leavesno_view_file_draws_its_own_rulepassing and onlythe_guard_reads_every_view_filefails — which is exactly the case the second test exists for.Sabotage — two, both confirmed
blame.rsdefines its ownfn divideragainno_view_file_draws_its_own_rule, naming the filethe_guard_reads_every_view_file— and the first test still passedScope
This settles the argument convention for the shared helpers only. The crate-wide split between
&PaletteandPaletteby value still exists in the widgets' own signatures. It is not what stood in the way here, and changing every signature is a different change.Screenshots
None retaken. Every rule is the same colour and thickness it already was —
palette.borderat 1px — so nothing on screen moved. The diff is 147 deletions against 169 insertions, most of the latter being the new module and its tests.Docs
UI_SPEC.md's Layout scale section records where a rule lives, both reasons they could not be shared before, and why the colour is not a parameter.Gate
cargo fmt --all -- --check,cargo clippy --workspace --all-targets --all-features --locked -- -D warnings,cargo test --workspace --all-features --locked— 898 tests, all green.