On Tue, 2015-08-18 at 09:13 +0200, Pavel Grunt wrote:
> On Tue, 2015-08-18 at 01:41 +0200, Fabiano Fidencio wrote:
> >
> >
> > On Mon, Aug 17, 2015 at 6:08 PM, Pavel Grunt <pgrunt@xxxxxxxxxx> wrote:
> > > On Mon, 2015-08-17 at 17:55 +0200, Fabiano Fidêncio wrote:
> > > > Coverity says:
> > > > Result is not floating-point (UNINTENDED_INTEGER_DIVISION)
> > > > interger_division: Dividing integer expressions "preferred->width * 100"
> > > > and "zoom", and then converting the integer quotient to type double. Any
> > > > remainder, or fractional part of the quotient, is ignored.
> > >
> > > I think it is better to remove the round(), otherwise you are changing the
> > > behavior (which is there since 33614f86db490364339ef69e0eb76f98a4ac8138).
>
> Hey Fabiano,
> I just suggested a way how to fix the coverity warning without changing the
> behavior.
> > I don't see why I am (wrongly) changing the behavior, Pavel.
> > Can you give me an example?
>
> you gave the example, sometimes there will be an extra pixel, so I was afraid it
> can trigger some extra size allocation events. See
> virt_viewer_display_size_allocate() in virt-viewer-display.c and
> virt_viewer_display_spice_size_allocate() virt-viewer-display-spice.c and
> virt_viewer_session_on_monitor_geometry_changed() in virt-viewer-session.c
>
> virt_viewer_session_on_monitor_geometry_changed() will be called if
> virt_viewer_display_get_preferred_size() != GtkAllocation, and the requested
> size will be set using virt_viewer_display_get_preferred_monitor_geometry()
>
> ACK if you test it and it is ok (no extra size request). It is also possible
> that you are avoiding the extra size request.
So I spent a little time testing and reading some related code. My main
concern was whether simply changing the zoom level would result in a
resize of the guest display. It turns out that this doesn't happen
because this function is not called in response to zoom changes (see the
early return in zoom_level_changed). So I think that the only time this
function is really called is after the window has been resized by the
user and we're about to send a new configuration down to the server. So
whether the value is rounded up or down at this point probably doesn't
make a lot of difference. I think it's probably best to use the more
"correct" approach. Additional testing would always be useful though.
Jonathon
>
> >
> > Nowadays, using the round or not using the
> > round would end up in the same result and I don't think it's the
> > expected/correct behavior.
> >
> > Let's consider: width = 640, NORMAL_ZOOM_LEVEL = 100, zoom = 85.
> >
> > Current behavior:
> > width = round (640*100/85)
> > width = round (752)
> > width = 752.
> >
> > Removing the round:
> > width = 640*100/85
> > width = 752
> >
> > What I consider the expected behavior:
> > width = round (640*100/85)
> > width = round (752.94)
> > width = 753
> >
> >
> > > Or is the rounding necessary ?
> > I do think so.
> >
> Thanks, it wasn't clear for me that it is about fixing some problem (is there a
> "black bar"? - it should be with your example).
>
> Pavel
>
> > >
> > > Pavel
> > > > ---
> > > > src/virt-viewer-display.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/virt-viewer-display.c b/src/virt-viewer-display.c
> > > > index 3efe24c..8431ae4 100644
> > > > --- a/src/virt-viewer-display.c
> > > > +++ b/src/virt-viewer-display.c
> > > > @@ -819,8 +819,8 @@ void
> > > > virt_viewer_display_get_preferred_monitor_geometry(VirtViewerDisplay*
> > > self,
> > > > if (virt_viewer_display_get_zoom(VIRT_VIEWER_DISPLAY(self))) {
> > > > guint zoom =
> > > > virt_viewer_display_get_zoom_level(VIRT_VIEWER_DISPLAY(self));
> > > >
> > > > - preferred->width = round(preferred->width * NORMAL_ZOOM_LEVEL /
> > > > zoom);
> > > > - preferred->height = round(preferred->height * NORMAL_ZOOM_LEVEL /
> > > > zoom);
> > > > + preferred->width = round(preferred->width * NORMAL_ZOOM_LEVEL /
> > > > (double) zoom);
> > > > + preferred->height = round(preferred->height * NORMAL_ZOOM_LEVEL /
> > > > (double) zoom);
> > > > }
> > > > }
> > > >
> > >
> >
> > Thanks for the review,
> > --
> > Fabiano Fidêncio
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
