Code review comment for lp:~brandontschaefer/unity/shutdown-dialog-hi-dpi

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

22 + RawPixel const DEFAULT_TEXTURE_SIZE = 168_em;

Mh, I don't like much the texture size defined here, considering they might change, but you can just do (without adding extra computation cost, since this will happen anyway)

nux::Size size;
auto const& texture_name = (texture_prefix + ".png");
gdk_pixbuf_get_file_info(texture_name.c_str(), &size.width, &size.height);
int max_size = std::max(std::round(size.width * scale), std::round(size.height * scale));
normal_tex_.Adopt(nux::CreateTexture2DFromFile(texture_name.c_str(), max_size, true));

Make sure you intialize the scale in SessionButton.

184 + auto* button = (Button*)area;

A static_cast would be nicer

145 + UpdateEMConverter();

I think you only need UpdateViewSize there, since the mode don't affect the monitor, but indeed might change the contents.

161 +void View::UpdateEMConverter()
162 +{
163 + int mouse_monitor = UScreen::GetDefault()->GetMonitorWithMouse();
164 + if (monitor != mouse_monitor)
165 + {
166 + monitor = mouse_monitor;
167 + UpdateViewSize();
168 + }
169 +}

I think you should get rid of this, just calling UpdateViewSize when the em converter changes.

Also, monitor variable should not be set by SessionView but by SessionController.
The view should not care about positioning itself.

219 + button->scale = cv_->DPIScale();
220 + button->scale = cv_->DPIScale();

Duplicated?

Finally, from a test run I'm getting these issues:
1) the title/subtitle text is not resized here (I don't see why, since it seems you're setting the scale properly)
2) Changing the scale value, when the view is hidden doesn't make it to be update once I request a new shutdown.

review: Needs Fixing

« Back to merge proposal