-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extract rendering strategy for decorations #3706
Conversation
Note that while this is a stable intermediate form (and can be merged), it doesn't extract everything that should be customised by a "decoration strategy": There's still two customisation points needed: the "static geometry", and the button placement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial feedback but it looks like a good move to me!
auto msd::RendererStrategy::default_strategy(std::shared_ptr<StaticGeometry const> const& static_geometry) | ||
-> std::unique_ptr<RendererStrategy> | ||
{ | ||
return std::make_unique<::RendererStrategy>(static_geometry); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if this had a header file that exposed a class called DefaultRendererStrategy
. I can see a world where users might want to extend the default in a minor way. Not a blocking suggestion though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just wires things up. It goes away in the follow-up PR
std::vector<std::string> usable_search_paths; | ||
for (auto const& path : font_path_search_paths) | ||
{ | ||
if (boost::filesystem::exists(path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use std::filesystem
instead now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is existing code I didn't aim to rework for this PR
for (auto const& path : usable_search_paths) | ||
{ | ||
auto const full_font_path = path + '/' + prefix + '/' + font.filename; | ||
if (boost::filesystem::exists(full_font_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::filesystem here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is existing code I didn't aim to rework for this PR
return (size.width > geom::Width{} && size.height > geom::Height{}) | ||
? size.width.as_int() * size.height.as_int() | ||
: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this check even necessary? Would 0
times anything just be 0
anyway...? Or is that std::nullopt or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is existing code I didn't aim to rework for this PR
|
||
if (!library || !face) | ||
{ | ||
mir::log_warning("FreeType not initialized"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like an error and not a warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is existing code I didn't aim to rework for this PR
} | ||
catch (std::runtime_error const& error) | ||
{ | ||
mir::log_warning("%s", error.what()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like an error and not a warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is existing code I didn't aim to rework for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving since this is all preexisting :)
This extracts the generic rendering integration from the default "strategy" for rendering