PM-14333 Complete fix for crash caused by spannable text creation #4479
Conversation
dseverns-livefront
requested review from
brian-livefront,
david-livefront,
ahaisting-livefront and
a team
as code owners
| it.getLinkAnnotations(expectedStart, expectedEnd) | ||
| .forEach { annotationRange -> | ||
| val linkAnnotations = it.getLinkAnnotations(expectedStart, expectedEnd) | ||
| if (linkAnnotations.isEmpty()) { |
There was a problem hiding this comment.
@phil-livefront this not existing is why that one test was passing no matter what.
|
No New Or Fixed Issues Found |
| highlights = listOf(email), | ||
| highlightStyle = SpanStyle( | ||
| val descriptionAnnotatedString = R.string.we_sent_an_email_to.toAnnotatedString( | ||
| email, |
There was a problem hiding this comment.
🔨 do we need internal param names here?
There was a problem hiding this comment.
weird vararg thing. if I put args = I need to pass in as arrayOf(email)
| val start = spannableBuilder.getSpanStart(annotation) | ||
| val end = spannableBuilder.getSpanEnd(annotation) | ||
| when (annotation.key) { | ||
| "emphasis" -> { |
There was a problem hiding this comment.
🔨 should this be a enum?
There was a problem hiding this comment.
I had them, but it basically served no other purpose than doing something like:
when (val enum = EnumType.valueOf(annotation.key.uppercase())) {
OPTION1 -> { ... }
and if annotation.key was not an enum value it would crash. I guess maybe that would be a good thing as that type of error is something that should be caught while testing so would be useful to the developer to be like hey you used an annotation we weren't ready for...and now I am thinking we should add as an enum :)
| this.replace( | ||
| this.getSpanStart(annotation), | ||
| this.getSpanEnd(annotation), | ||
| args[argIndex], |
There was a problem hiding this comment.
can we add a condition to make sure the index exists here?
| val annotations = getSpans<Annotation>() | ||
| annotations | ||
| .filter { | ||
| it.key == "arg" |
There was a problem hiding this comment.
not sure how ya'll do this stuff yet, but thoughts on adding a string constants model for the hardcoded strings? not sure these would ever change though so might be a silly ask haha. my first instinct when i see hardcoded strings tho is to ask about them haha
dseverns-livefront
force-pushed
the
PM-14333-add-back-functionalitity
branch
from
a58e112 to
76d4ac5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4479 +/- ##
==========================================
- Coverage 88.93% 88.89% -0.05%
==========================================
Files 459 460 +1
Lines 39839 39754 -85
Branches 5634 5643 +9
==========================================
- Hits 35432 35340 -92
- Misses 2440 2443 +3
- Partials 1967 1971 +4 ☔ View full report in Codecov by Sentry. |
… strings so if the full string is not translated but the spanned words are they will be highlighted and functional
dseverns-livefront
force-pushed
the
PM-14333-add-back-functionalitity
branch
from
918ff60 to
4c9423f
Compare
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-14333
📔 Objective
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes