Repository navigation
Add commit message to menu - #625
Conversation
This avoids some misleading understanding of what is the concerning commit
26211d6 to
f0c7bb3
Compare
|
Screenshot differs: emulatorApi=arm64 with 4 screenshot(s) |
goneng
left a comment
goneng
left a comment
There was a problem hiding this comment.
Nice change, the bare "Checkout Commit" has confused me too.
Three things I ran into while reading it:
-
13 characters may be too short to disambiguate.
After the...indicator only 10 characters of the subject survive.
This means half of the commits will just showMerge pull...,
so for the most common commit-type in this repo the new title
does not narrow anything down.
At 40 characters the collisions drop to 15 of 400.
Could we raise the limit? -
The cut can split a surrogate pair.
PBNSStringTruncateModeEndcuts at a UTF-16 index without looking at
grapheme boundaries, so a subject likeFix this <emoji> thing that broke
truncates to a string that cannot be encoded as UTF-8 and shows up as a
broken glyph in the menu.
AppKit tolerates it, so it is corruption rather than a crash.
This is a latent bug inNSString_Truncatefrom 2010 and not something
this PR introduced, but grep finds no other caller in the tree, so this is
the change that first exposes it.
Backing the cut off to a grapheme boundary should be enough:
NSUInteger cut = total - indicator.length;
NSRange seq = [s rangeOfComposedCharacterSequenceAtIndex:cut];
if (seq.location < cut) cut = seq.location; -
Commits with an empty message render as (null).
subject returnsmessageSummary, which isnilafter
git commit --allow-empty-message, so the item readsCheckout Commit "(null)".
Maybe catch that and return an empty-string in this case.
1. Extended to 40 chars: truncateToLength:43 (40 + 3 for ...) 2. Empty message handling: falls back to NSLocalizedString(@"<empty message›", ...) when firstCommit. subject is empty, instead of ever formatting a nil into (null). 3. Grapheme-safe truncation: helpers applied to all three truncation modes (Center, Start, End), so a cut index that would split a surrogate pair or other multi-unit grapheme backs off/pushes forward to drop the whole sequence instead of corrupting it. Verified with an emoji-containing string across a range of lengths — output is always valid UTF-8, and the emoji is either fully kept or fully dropped, never split.
|
I applied your suggestions and added some tests |
|
Btw, this screenshot shows already the new feature But this screenshot compare feature is current not perfect.... |
|
Screenshot differs: emulatorApi=arm64 with 4 screenshot(s) |
|
Thanks, the grapheme helpers are nicer than what I suggested :) One thing worth fixing: I reverted the fix and re-ran the new tests to check they catch the bug, and saw that I think that raising the loop to Two minor ones, take or leave:
Rest looks great to me. |
I'll split it |
This avoids some misleading understanding of what is the concerning commit.
Follow up of #406
previous:
new: