Closed Bug 732733 Opened 8 years ago Closed 4 years ago

Scale drag images down to thumbnails

Categories

(Core :: Widget, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jimm, Assigned: mb, Mentored)

References

()

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(4 files, 12 obsolete files)

591.05 KB, image/png
Details
306.09 KB, image/png
Details
39.40 KB, image/jpeg
Details
8.15 KB, patch
jimm
: review+
Details | Diff | Splinter Review
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.
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?
Attached image screenshot (obsolete) —
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.
Attached image screenshot
with a drop folder, which gives a better feel for the problem.
Attachment #602671 - Attachment is obsolete: true
Attached image chrome
Chrome uses a better size thumbnail, although I think the image should also be dimmed a bit.
Mentor: jmathies
Whiteboard: [good first bug][lang=c++]
Newcomer here. I'm interested in this bug if it hasn't been resolved elsewhere. Could someone point me to the pertinent file?
Flags: needinfo?(jmathies)
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.
Assignee: nobody → aslakamsani
Flags: needinfo?(jmathies)
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"?
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
What you could also do is store the initial thumbnail dims in prefs. :) That way people can tweak their thumbnail size.
(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?
> 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.
Attached patch Limits drag image to 512x512. (obsolete) — Splinter Review
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.
Attachment #8500036 - Flags: review?(jmathies)
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.
Attachment #8500036 - Flags: review?(jmathies) → review-
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.
(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?
Flags: needinfo?(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.
Flags: needinfo?(dao)
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.
Review please and advise what I can do better.
Thanks
Attachment #8730309 - Flags: review?(jmathies)
Attachment #8730309 - Attachment is obsolete: true
Attachment #8730309 - Flags: review?(jmathies)
Attachment #8730315 - Flags: review?(jmathies)
Assignee: aslakamsani → moritzbrunner
Attachment #8500036 - Attachment is obsolete: true
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);
Attachment #8730315 - Flags: review?(jmathies) → review-
Attached image tabs.jpg
Is the patch now ok?
Attachment #8730315 - Attachment is obsolete: true
Attachment #8735109 - Flags: review?(jmathies)
review ping?
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
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?
Attachment #8735109 - Flags: superreview?(dao)
Attachment #8735109 - Flags: review?(jmathies)
Attachment #8735109 - Flags: review+
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.
Attachment #8735109 - Flags: superreview?(dao)
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.
Sorry, I deleted 'resize' because of comment 20 and oversaw that it is used some lines below, while I cleaned the patch up.
Attachment #8735109 - Attachment is obsolete: true
Attachment #8738263 - Flags: review?(jmathies)
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.
Attachment #8738263 - Flags: review?(jmathies)
This patch fixes the text dragging problem of the previous version and fix for whole-site-selection-dragging-problem will follow.
Attachment #8738263 - Attachment is obsolete: true
Is the patch now ok?
Attachment #8739531 - Attachment is obsolete: true
Attachment #8748172 - Flags: feedback?(enndeakin)
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
Flags: needinfo?(moritzbrunner)
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.
I'm sorry for the messed up version.
Attachment #8748172 - Attachment is obsolete: true
Attachment #8748172 - Flags: feedback?(enndeakin)
Flags: needinfo?(moritzbrunner)
Attachment #8748674 - Flags: feedback?(enndeakin)
Attachment #8748674 - Attachment is obsolete: true
Attachment #8748674 - Flags: feedback?(enndeakin)
Sorry uploaded old versions of the patch
Attachment #8748685 - Flags: feedback?(jmathies)
Attachment #8748685 - Flags: feedback?(enndeakin)
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.
Attachment #8748685 - Flags: feedback?(enndeakin) → feedback+
You are right, it isn't a platform specific bug.
Should the platform be changed to 'All' or 'Unspecified'?
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.
Attachment #8748685 - Flags: feedback?(jmathies) → feedback-
Component: Widget: Win32 → Widget
OS: Windows 7 → All
Is it now ok?
Attachment #8748685 - Attachment is obsolete: true
Attachment #8749117 - Flags: feedback?(jmathies)
Summary: On windows, scale drag images down to thumbnails → Scale drag images down to thumbnails
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?
Attachment #8749117 - Flags: feedback?(jmathies) → feedback-
Now ok?
Attachment #8749117 - Attachment is obsolete: true
Attachment #8750007 - Flags: feedback?(jmathies)
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 (.
Attachment #8750007 - Flags: feedback?(jmathies) → feedback+
Attachment #8750007 - Attachment is obsolete: true
Attachment #8752194 - Flags: review?(jmathies)
Attachment #8752194 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34193aa198ac
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1315660
You need to log in before you can comment on or make changes to this bug.