Bug 1884816 - Expose icon information for suggestion as struct #6162
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6162 +/- ##
=======================================
Coverage 84.09% 84.09%
=======================================
Files 117 117
Lines 15627 15627
=======================================
Hits 13141 13141
Misses 2486 2486 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Nice, thanks!
Naming nit: Can we use data instead of content everywhere please? data is more natural, it matches the column name in the DB, and it's shorter.
Smaller naming nit: It would be nice to consistently use mimetype or mime_type, but no big deal I guess. It's two words, but I don't think we should change the DB schema just for that. I wonder if there's a Rust convention for one over the other?
components/suggest/src/db.rs
Outdated
| let icon = match row.get("icon_content")? { | ||
| Some(content) => Some(SuggestionIcon { | ||
| content, | ||
| mime_type: row.get("icon_mimetype")?, | ||
| }), | ||
| _ => None, | ||
| }; |
There was a problem hiding this comment.
Nit: I would write this using Option::map(), but I'm not sure it's better and Lina might disagree:
let icon_content = row.get::<_, Option<_>>("icon_content")?;
let icon_mimetype = row.get::<_, Option<_>>("icon_mimetype")?;
let icon = icon_content.map(|content| SuggestionIcon {
content,
mime_type: icon_mimetype.unwrap_or_else(|| "".into()),
});
components/suggest/src/yelp.rs
Outdated
| Ok(match result { | ||
| (Some(content), Some(mime_type), score) => { | ||
| (Some(SuggestionIcon { content, mime_type }), score) | ||
| } | ||
| _ => (None, result.2), | ||
| }) |
There was a problem hiding this comment.
Similar to above, I would use map():
let (icon_content, icon_mimetype, score) = self.conn.query_row_and_then_cachable(
// ...
)?;
let icon = icon_content.map(|content| SuggestionIcon {
content,
mime_type: icon_mimetype.unwrap_or_else(|| "".into()),
});
Ok((icon, score))
|
Thank you very much, @0c0w3 !
Okay!
Yeah, I also thought about the name consistency.. So, I will use |
There was a problem hiding this comment.
This looks wonderful, thank you @dadaa! I had a small preference for mime_type over mimetype, but don't feel strongly about it—please feel free to keep mimetype if you prefer.
We're going to need breaking change PRs on Android (and tests) and iOS, though. I added a section to the readme about them, but I'd be happy to walk you through the process!
Please make sure to also add a CHANGELOG.md entry for this breaking change. (You might need to add a "Breaking Changes" section for v125.0; check out this example).
components/suggest/src/suggestion.rs
Outdated
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct SuggestionIcon { | ||
| pub data: Vec<u8>, | ||
| pub mimetype: String, |
There was a problem hiding this comment.
Hah, I was just about to suggest mime_type, and then saw you renamed it for consistency with the schema.
I think mime_type looks a little cleaner (it becomes mimeType in JS, Swift, and Kotlin; mimetype there almost looks like a typo). I wouldn't worry too much about keeping it consistent with the schema—we already have other fields that use different names!
There was a problem hiding this comment.
Okay! I will change to mime_type.
components/suggest/src/db.rs
Outdated
| let icon_mimetype = row.get::<_, Option<_>>("icon_mimetype")?; | ||
| let icon = icon_data.map(|data| SuggestionIcon { | ||
| data, | ||
| mimetype: icon_mimetype.unwrap_or_else(|| "".into()), |
There was a problem hiding this comment.
| mimetype: icon_mimetype.unwrap_or_else(|| "".into()), | |
| mimetype: icon_mimetype.unwrap_or_default(), |
I think this should work; please ignore if it doesn't!
@linabutler
Am I right? |
https://bugzilla.mozilla.org/show_bug.cgi?id=1884816
Pull Request checklist
[ci full]to the PR title.Branch builds: add
[firefox-android: branch-name]to the PR title.