732733 - Scale drag images down to thumbnails

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Jim Mathies [:jimm]]

Firefox doesn't scale dragged image, so if you drag a very large image, it consumes your whole desktop making it very hard to find your drop location. We should scale these images down to thumbnails when needed. There was some discussion on where this fix should be implemented in the original drag image bug 374593.

Both IE and Chrome correctly scale.

Comment 1

[Neil Deakin]

The current code should be scaling large images down to no more than half the screen size. Are you saying this isn't happening, or that some other behaviour is desired?

Comment 2

[Jim Mathies [:jimm]]

Attachment: screenshot

We scale the image, I just think we aren't scaling it down enough. The overlay image overwhelms making it hard to find the drop target.

Comment 3

[Jim Mathies [:jimm]]

Attachment: screenshot

with a drop folder, which gives a better feel for the problem.

Comment 4

[Jim Mathies [:jimm]]

Attachment: chrome

Chrome uses a better size thumbnail, although I think the image should also be dimmed a bit.

Comment 5

[Amit S. Lakamsani [:asl]]

Newcomer here. I'm interested in this bug if it hasn't been resolved elsewhere. Could someone point me to the pertinent file?

Comment 6

[Jim Mathies [:jimm]]

Back when I put together drag images (bug 374593), I had some code that scaled these down to thumbnail size, which I've always preferred to a full size drag images. Per comment 1 here, there was a discussion in bug 374593 about what code should be scaling these to thumbnails. See comments 56 through 61. We need to investigate that code, and try to figure out what the current scaling behavior is. Then we can look at tweaking it for windows so we get thumbnails instead of large images.

You could also put together a good drag image test page we can test with and post it here. :) good luck! You can ni me if you have further questions.

Comment 7

[Amit S. Lakamsani [:asl]]

I may be thinking aloud here.

I did some digging and found that PresShell::PaintRangePaintInfo() and nsBaseDragService::DrawDragForImage() use the same scaling algorithm, where the dragged image will never be larger than a quarter of the available screen. I presume the scaling change mentioned here refers to nsBaseDragService alone, unless I ought to change the other one as well (they *are* very similar)?

In either case, I'm not sure what to use as a reference for thumbnail size. It would be quite easy to just limit the dimensions of the images from being half the screen size to say, a quarter, and thus create drag images of one sixteenth the size of the screen. Should I just go for this, or should I use a better definition of thumbnail size other than "significantly smaller than it is now"?

Comment 8

[Jim Mathies [:jimm]]

(In reply to Amit S. Lakamsani [:asl] from comment #7)
> I may be thinking aloud here.
> 
> I did some digging and found that PresShell::PaintRangePaintInfo() and
> nsBaseDragService::DrawDragForImage() use the same scaling algorithm, where
> the dragged image will never be larger than a quarter of the available
> screen. I presume the scaling change mentioned here refers to
> nsBaseDragService alone, unless I ought to change the other one as well
> (they *are* very similar)?

I'm not sure, can you answer this? You can try disabling either in a test build to see how it changes the size of large drag images.

I was just testing on windows and mac, looks like currently the resulting drag image is about 2.1x the width of the screen and 2x the height.


> In either case, I'm not sure what to use as a reference for thumbnail size.
> It would be quite easy to just limit the dimensions of the images from being
> half the screen size to say, a quarter, and thus create drag images of one
> sixteenth the size of the screen. Should I just go for this, or should I use
> a better definition of thumbnail size other than "significantly smaller than
> it is now"?

I think picking a reasonable icon size is good, I don't see a reason to adhere to any kind of system setting. Maybe 512x512 max? Also, we should investigate how dpi plays into this, we might want to multiply the dims by the current desktop dpi.

Comment 9

[Jim Mathies [:jimm]]

What you could also do is store the initial thumbnail dims in prefs. :) That way people can tweak their thumbnail size.

Comment 10

[Amit S. Lakamsani [:asl]]

(In reply to Jim Mathies [:jimm] from comment #8)
> (In reply to Amit S. Lakamsani [:asl] from comment #7)
> I was just testing on windows and mac, looks like currently the resulting
> drag image is about 2.1x the width of the screen and 2x the height.

I looked up some wallpapers that have the same dimensions as my screen, and dragging them takes up only a quarter of my screen. Did you mean to say that the screen width and height were twice those of the image?

Comment 11

[Jim Mathies [:jimm]]

> I was just testing on windows and mac, looks like currently the resulting
> drag image is about 2.1x the width of the screen and 2x the height.

Sorry, I meant 1/2.1=.47 the width, and .5 the height. :) I was measuring using the drag image.

Comment 12

[Amit S. Lakamsani [:asl]]

Attachment: Limits drag image to 512x512.

Sorry for my absence, I ought to have made this patch a while ago.

The patch does limit the drag image appropriately (although I think perhaps the thumbnail should be smaller). As for DPI, it seems that that is handled here: [http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5082], just before my changes, although I'm not sure. Please do advise me on how to proceed.

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8500036 [details] [diff] [review]
Limits drag image to 512x512.

The '512' values in this code are bad form, they should be assigned to a const or define declared at the top of the page and then references here. However, I think you can go one step further and make this much more useful - store the the maxHeight, maxWidth values in new prefs. If they aren't defined, we fall back on the old behavior. In firefox.js prefs file, we can set this to 512 for Windows.

Comment 14

[Dão Gottwald [:dao]]

I'm not sure we should have hidden prefs for this in the first place, but this would belong in all.js, not firefox.js.

Comment 15

[Jim Mathies [:jimm]]

(In reply to Dão Gottwald [:dao] from comment #14)
> I'm not sure we should have hidden prefs for this in the first place, but
> this would belong in all.js, not firefox.js.

I thought it would be pretty neat if users could define the size of their drag thumbnails from content. Would you prefer the hard coded values?

Comment 16

[Dão Gottwald [:dao]]

I think we should hardcode by default. If people file bugs about this, saying that the hardcoded value doesn't work for them for some reason, we can always add prefs later.

Comment 17

[Moritz Brunner [:mb]]

Hi,
I'm a newbie and want to fix this bug as my first bug, is that ok?
PS:I never fixed a bug in a open source project before.

Comment 18

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

Review please and advise what I can do better.
Thanks

Comment 19

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 8730315 [details] [diff] [review]
Resize Drag Image relative to screensize.patch

Review of attachment 8730315 [details] [diff] [review]:
-----------------------------------------------------------------

I like where this is headed. Lets start with some cleanup work.

::: layout/base/nsPresShell.cpp
@@ +4868,5 @@
>    // use the rectangle to create the surface
>    nsIntRect pixelArea = aArea.ToOutsidePixels(pc->AppUnitsPerDevPixel());
>  
> +  // define the scalefactor relative to the max screen height/width
> +  #define RELATIVESCALEFACTOR 0.0625f

I think I'd prefer a slightly larger value here, maybe use 0.0925f?

Also, lets move this define up to the top of the file with the comment explaining what it applies to.

@@ +4873,5 @@
> +
> +  // if the image should not be resized, the scale, relative to the original image, must be 1
> +  float scale = 1.0;
> +  nsIntRect rootScreenRect =
> +	  GetRootFrame()->GetScreenRectInAppUnits().ToNearestPixels(

see attached editor capture - you have tabs in your patch. Mozilla code always uses spaces for indentation. So clean up your patch by removing any tabs.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +4887,1 @@
>    if (resize) {

No need to break this up into two lines, use:

if (aFlags & RENDER_AUTO_SCALE) {
...

@@ +4887,4 @@
>    if (resize) {
> +    // resize image relative to the screensize
> +    // get best width relative to screensize and get the scale to reach this size
> +	  scale = (float(maxWidth)*RELATIVESCALEFACTOR) / float(pixelArea.width);

nit - 

scale = float(maxWidth) * RELATIVESCALEFACTOR / float(pixelArea.width);

@@ +4887,5 @@
>    if (resize) {
> +    // resize image relative to the screensize
> +    // get best width relative to screensize and get the scale to reach this size
> +	  scale = (float(maxWidth)*RELATIVESCALEFACTOR) / float(pixelArea.width);
> +	  const float worstheight = float(pixelArea.height)*scale;

nit - 'worstHeight'

@@ +4894,5 @@
> +    the height when the width is perfect, and the best height
> +    and get the difference between them to divide it by 2 and
> +    to get the best compromise-height by adding this offset to the worst height.
> +    Now it computes the scale to reach this height and
> +    now the width and height combination is the best compromise*/

This comment needs some cleanup, it rambles too much. Try and break this down into clear and concise steps, and move the comments to the areas of the code where the steps occur.

@@ +4896,5 @@
> +    to get the best compromise-height by adding this offset to the worst height.
> +    Now it computes the scale to reach this height and
> +    now the width and height combination is the best compromise*/
> +
> +	scale = ((worstheight)+(float(maxHeight)*RELATIVESCALEFACTOR - worstheight) / 2) / (float(pixelArea.height));

for clarity sake lets break this up somewhat. There's no performance penalty since optimizing compilers will meld this together into the fastest path.

float bestHeight = float(maxHeight)*RELATIVESCALEFACTOR;
float difference = bestHeight - worstHeight;
scale = ((worstHeight + difference) / 2) / float(pixelArea.height);

Comment 21

[Jim Mathies [:jimm]]

Attachment: tabs.jpg

Comment 22

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

Is the patch now ok?

Comment 23

[Moritz Brunner [:mb]]

review ping?

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 8735109 [details] [diff] [review]
Resize Drag Image relative to screensize.patch

Code looks better now. 

I would like to land this because I think thumbnails for drag images are superior to large fullscale drag image, it's easier to tell where you're going to drop the file. Dao, any reservations about making this change?

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8735109 [details] [diff] [review]
Resize Drag Image relative to screensize.patch

WFM. If you have reservations about possible UX downsides, please request ui-review.

Comment 26

[Neil Deakin]

This patch doesn't build for me. 'resize' isn't defined.

Does this also affect selection drags? It looks like it would but it shouldn't and Chrome does not rescale them.

Comment 27

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

Sorry, I deleted 'resize' because of comment 20 and oversaw that it is used some lines below, while I cleaned the patch up.

Comment 28

[Neil Deakin]

I just tried this patch, and image drags are ok, but selection drags aren't sized properly. For example:

1. Select a word. Drag it and the drag feedback is twice as large as it should be.
2. Select all. The drag feedback is shrunk down. It should not be.

Comment 29

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

This patch fixes the text dragging problem of the previous version and fix for whole-site-selection-dragging-problem will follow.

Comment 30

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

Is the patch now ok?

Comment 31

[Jim Mathies [:jimm]]

Looks like some other work leaked into your patch, I see changes to

media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_frame_win.cc
media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_frame_win.h
python/mozbuild/mozbuild/backend/visualstudio.py

Comment 32

[Jim Mathies [:jimm]]

Comment on attachment 8748172 [details] [diff] [review]
Resize Drag Image relative to screensize.patch

Review of attachment 8748172 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsBaseDragService.cpp
@@ +488,5 @@
>                              int32_t aScreenX, int32_t aScreenY,
>                              nsIntRect* aScreenDragRect,
>                              RefPtr<SourceSurface>* aSurface,
> +                            nsPresContext** aPresContext,
> +                            char ImageDrag)

use uint32_t here, which matches what nsPresShell uses for these flag values.

Comment 33

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

I'm sorry for the messed up version.

Comment 34

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

Sorry uploaded old versions of the patch

Comment 35

[Neil Deakin]

Comment on attachment 8748685 [details] [diff] [review]
Resize Drag Image relative to screensize.patch

This works better for selections and image drags. I didn't look at the actual patch though. I tested on Mac. I assume that this isn't actually a Windows specific bug and that the bug title and platform should be modified.

Comment 36

[Moritz Brunner [:mb]]

You are right, it isn't a platform specific bug.
Should the platform be changed to 'All' or 'Unspecified'?

Comment 37

[Jim Mathies [:jimm]]

Comment on attachment 8748685 [details] [diff] [review]
Resize Drag Image relative to screensize.patch

Review of attachment 8748685 [details] [diff] [review]:
-----------------------------------------------------------------

Getting there! Lots of feedback below, please needinfo me if you have any questions.

::: widget/nsBaseDragService.cpp
@@ +633,5 @@
>      nsIntRegion clipRegion;
>      if (aRegion) {
>        aRegion->GetRegion(&clipRegion);
>      }
>  

Lets declare the variable that holds the nsIPresShell render type here and lets set a default value such that:

value = mImage ? 0 : nsIPresShell::RENDER_AUTO_SCALE;

@@ +635,5 @@
>        aRegion->GetRegion(&clipRegion);
>      }
>  
> +    nsCOMPtr<nsIDOMNode> child;
> +    nsCOMPtr<nsIDOMNodeList> childlist;

nit - childList

@@ +636,5 @@
>      }
>  
> +    nsCOMPtr<nsIDOMNode> child;
> +    nsCOMPtr<nsIDOMNodeList> childlist;
> +    bool isimage = 0;

nit - isImage

@@ +639,5 @@
> +    nsCOMPtr<nsIDOMNodeList> childlist;
> +    bool isimage = 0;
> +    uint32_t length;
> +    uint32_t count = 0;
> +    nsAutoString childnodename;

nit - childNodeName

@@ +641,5 @@
> +    uint32_t length;
> +    uint32_t count = 0;
> +    nsAutoString childnodename;
> +
> +    nsresult rv = dragNode->GetChildNodes(getter_AddRefs(childlist));

lets fall through here on failure such that presShell->RenderNode gets called with the default value. So:

if (NS_SUCCEEDED(dragNode->GetChildNodes) &&
    NS_SUCCEEDED(childlist->GetLength(&length)) {
  while (count < length) {
    if (NS_SUCCEEDED(childlist->Item(count, getter_AddRefs(child)) &&
      ..
        ..
          isImage = 1;
    }
  }
}

If mImage is valid we don't need to do this step correct? So we should check that first and skip past this iteration when we can.

Comment 38

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

Is it now ok?

Comment 39

[Jim Mathies [:jimm]]

Comment on attachment 8749117 [details] [diff] [review]
Resize Drag Image relative to screensize.patch

Review of attachment 8749117 [details] [diff] [review]:
-----------------------------------------------------------------

Getting there, just code formatting a flow at this point.

::: layout/base/nsPresShell.cpp
@@ +4995,3 @@
>    if (resize) {
> +  // check if image-resizing-algorithm should be used
> +  if (aFlags & RENDER_IS_IMAGE) {

nit - your indentation is off here. This block needs to be indented for the if (resize) { if block.

@@ +5011,5 @@
> +    // half the difference and add it to worstHeight,
> +    // then get scalefactor to reach this
> +    scale = (worstHeight + difference / 2) / float(pixelArea.height);
> +  }
> +  else {

nit - } else {

@@ +5021,5 @@
> +          // divide the maximum size by the image size in both directions. Whichever
> +          // direction produces the smallest result determines how much should be
> +          // scaled.
> +          if (pixelArea.width > maxWidth)
> +              scale = std::min(scale, float(maxWidth) / pixelArea.width);

nit - brace this if statement and the one below it.

::: widget/nsBaseDragService.cpp
@@ +674,5 @@
> +                break;
> +            }
> +            count++;
> +        }
> +    }

nsresult rv = dragNode->GetChildNodes(getter_AddRefs(childList));
// error handling
if (NS_FAILED(rv)) {
    error = 1;
}

Not a fan of this approach where you test for failure, set error, and then fall through and check error at each consecutive step. First, why even use 'error'? You could just check rv again. Also if someone adds additional processing and forgets to check 'error', bad things happen. Since we don't rely on renderFlags getting set here, we can clean this up a bit by checking for success instead and nesting logic:

if (NS_SUCCEEDED(dragNode->GetChildNodes(getter_AddRefs(childList))) &&
    NS_SUCCEEDED(childList->GetLength(&length))) {
    // check every childnode for being a img-tag
    while (count < length && !error) {
      if (NS_FAILED(childList->Item(count, getter_AddRefs(child))) ||
          NS_FAILED(child->GetNodeName(childNodeName))) {
        break;
      }
      if (childNodeName.LowerCaseEqualsLiteral("img")) {
        ...
      }
    }
  }
}

make sense?

Comment 40

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

Now ok?

Comment 41

[Jim Mathies [:jimm]]

Comment on attachment 8750007 [details] [diff] [review]
Resize Drag Image relative to screensize.patch

Review of attachment 8750007 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsBaseDragService.cpp
@@ +642,5 @@
> +      uint32_t length;
> +      uint32_t count = 0;
> +      nsAutoString childNodeName;
> +
> +      if (NS_SUCCEEDED(dragNode->GetChildNodes(getter_AddRefs(childList))) && NS_SUCCEEDED(childList->GetLength(&length))) {

nit - lets wrap this line, you can stack the two NS_SUCCEEDEDs here.

@@ +645,5 @@
> +
> +      if (NS_SUCCEEDED(dragNode->GetChildNodes(getter_AddRefs(childList))) && NS_SUCCEEDED(childList->GetLength(&length))) {
> +        // check every childnode for being a img-tag
> +        while (count < length) {
> +          if (NS_FAILED(childList->Item(count, getter_AddRefs(child))) || NS_FAILED(child->GetNodeName(childNodeName))) {

ditto, wrap me.

@@ +662,4 @@
>      nsIntPoint pnt(aScreenDragRect->x, aScreenDragRect->y);
>      *aSurface = presShell->RenderNode(dragNode, aRegion ? &clipRegion : nullptr,
> +      pnt, aScreenDragRect,
> +      renderFlags);

nit - this should be indented the way it was, over past the first (.

Comment 42

[Moritz Brunner [:mb]]

Attachment: Resize Drag Image relative to screensize.patch

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34193aa198ac

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34193aa198ac