context menus: add Copy Image per ui spec

RESOLVED FIXED

Status

SeaMonkey
UI Design
--
enhancement
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: sairuh (rarely reading bugmail), Assigned: O. Atsushi (Torisugari))

Tracking

({helpwanted})

Trunk
helpwanted
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P0], [adt3], URL)

Attachments

(7 attachments, 8 obsolete attachments)

2.56 KB, patch
Details | Diff | Splinter Review
6.66 KB, patch
Details | Diff | Splinter Review
4.20 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
1.68 KB, patch
Details | Diff | Splinter Review
15.64 KB, patch
Details | Diff | Splinter Review
10.82 KB, patch
Details | Diff | Splinter Review
15.45 KB, patch
Simon Fraser
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
noticed that with 2002.04.03.09-static mozilla bits on linux rh7.2, there's no
Copy Image context menu item. per Marlon's spec, the context menus for an inline
image, image as link, and image as url should contain this item.

however, i don't know if there's even a backend for copying image. if not, i'd
imagine there's not enough time to implement the backend, hence this could
prolly be futured.
(Reporter)

Updated

15 years ago
Keywords: nsbeta1
QA Contact: paw → sairuh
Summary: context menus: add Copy Image per spec → context menus: add Copy Image per ui spec
(Reporter)

Comment 1

15 years ago
...but i'm nominating for nsbeta1 if there *is* a backend. :)

Comment 2

15 years ago
Right. I don't think the backend has been implemented.

Comment 3

15 years ago
Nav triage team: nsbeta1-
Keywords: nsbeta1 → nsbeta1-
(Reporter)

Updated

15 years ago
Blocks: 135841

Comment 4

15 years ago
See bug 21747, "Implement backend for cmd_copyImageContents"

Comment 5

15 years ago
There are already 6 context menu items for an image, and I don't see how this
one would be useful.  Adding this would also make it harder to find "Copy Image
Location" because its name is similar.
Depends on: 21747
Whiteboard: [Hixie-P0]
No longer depends on: 21747

Comment 6

15 years ago
*** Bug 138652 has been marked as a duplicate of this bug. ***

Updated

15 years ago
OS: Linux → All
Hardware: PC → All

Comment 7

15 years ago
If this is bugging anybody else, I just found out I can drag-n-drop an
image onto another document (on Mac OS X, at least).  Not as convenient
as copy-paste, but better than having to save it somewhere.  Cheers.

Comment 8

15 years ago
This bug seems to be still open although a compremise for composer is great, but
the real thread I thought was just a simple image copy, right mouse click.

Comment 9

15 years ago
Instead of adding Copy Image to the context menu, I think we should make the
following work:
1. Drag an image or image link directly into an image editor.
2. Start dragging an image or image link, drop immediately, Ctrl+C, paste into
image editor.
3. Drag an image (but not an image link) to the desktop.

Comment 10

15 years ago
The Context Menu is a pretty much standard for objects, copy and paste.

It will be sadly missed. Dragging images around ang getting rogue copies of temp items 
is very poor UI.

I Vote to keep it under Context menu's COPY.

:-)

Comment 11

15 years ago
I vote for "Copy Image to Clipboard" or simply "Copy Image" from the context 
menu as well. Please do not confuse Joe Users with drag-n-drops and other foo. 
You could add those too if you want, but always have a direct "copy image" on 
the context menu as well.

Comment 12

15 years ago
I don't think we can have both Copy Image Location and Copy Image in the context
menu without making both commands hard to find.  I'm ok with removing Copy Image
Location and replacing it with Copy Image.

Comment 13

15 years ago
The "Copy Image Location" should just be a *selectable* text on the Properties 
menu when you right click to an image. It should not be part of the main 
context menu. IE has it right there, UI-wise.

Comment 14

15 years ago
I agree with symonty, it would be bad practice to depend on drag and drop as the
only method to carry images around.  drag and drop isn't immediately evident to
some types of users, especially on windows.

as for eliminating [Copy Image Location] - i don't see why this feature
justifies the removal of another which is completely different.   the spec shows
[Copy Image], the more frequently used item, is above [Copy Image Location]
giving it priority.  If we think [Copy Image Location] is a waste of space, lets
discuss it in another bug.  Keep in mind it is maintaining 4.x parity.  It runs
parallel with other menus items such as [Copy Link Location], [Copy Email
Address] etc..  Wouldn't the removal of one justify the removal of these others,
since they offer the same level of utility?  The contextual menus were
predicated on providing means to manipulate (copy) underlying data not
immediately visible, nor obtainable without two or three additional steps.   
If Copy Image adds two flavours to the clipboard -- the image itself, and the
URI -- then the one menu item can perform both actions in a context sensitive
manner.

Comment 16

15 years ago
that is definitely an option, but what about the case when an application could
receive both flavours?  do we give image priority over URI, wherever both are
accepted?  

i think it's acceptable, and am willing to bet that the image is going to be the
desired result in the majority of cases where applications could receive both
data types. therefore, for the minority, ambiguous case (when URI is desired),
users are forced to live with the tradeoff of having quick and concise context
menus, instead of total unfettered control.  and the former is our ultimate concern.
Yeah, for the cases where they accept both, the user just has to use Windows'
"Paste Special" command (and whatever the equivalent is on other operating
systems). Or they can just View Image and copy the URI from the Location bar...
(Reporter)

Comment 18

15 years ago
nominating. any time for this for buffy?
Keywords: nsbeta1- → nsbeta1

Comment 19

15 years ago
I'm ok with replacing Copy Image Location with Copy Image.  What should we do 
for Linux, where Mozilla doesn't yet have the backend for copying an image (bug 
21747)?
Depends on: 21747

Comment 20

15 years ago
nsbeta1+/adt3 per the nav triage team.
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [Hixie-P0] → [Hixie-P0], [adt3]

Comment 21

15 years ago
The "Copy Image" feature is one I use very frequently in Internet Explorer and
one I sorely miss in Mozilla.  If it comes down to a choice between "Copy Image
Location" and "Copy Image", I would much prefer to have "Copy Image".  This will
give me one less reason to open up IE.  :)

Comment 22

15 years ago
I totally agree with that, I open IE or Opera just to have the copy image
feature. It's a shame to not have such a handy feature.
Blocks: 178998
*** Bug 184426 has been marked as a duplicate of this bug. ***

Comment 24

15 years ago
I strongly request this feature be put in.  The ONLY reason I ever open up IE
anymore is to be able to copy images directly to the clipboard!  I really do not
feel it will junk up the pop-up UI at all.

Comment 25

15 years ago
I would have to strongly agree with the above comments on this to put this
feature in. This is still one of the only reasons I still use IE (and one of the
reasons keeping my IE-using friends away from Moz/Phoenix/etc...)

As an engineering student with a lot of need to do web-based research, this
feature is a godsend to capturing images and placing them directly into
wordprocessing apps. 

After all, lets not forget the main purpose of the web is still for information
gathering. This is a much better solution than to "Save Image as..." and then
opening it up again in Word,etc.

Also, as for the comments that state that this would add more clutter to the
context menu. I have a hard time justifying the use of the "Send Image..."
selection. Any decent mail app will be able to copy and paste an image quite
easily. No reason to have a selection that does the SAME thing but losing
important (and basic UI) functionality.

Thanks!

Comment 26

15 years ago
This feature is especially useful for grapic artists who use Photoshop or 
other package to modify, combine and other wise use images from the web.  I 
have to keep boot IE every time I want to grab images from a website, and 
paste them into my graphics app.

Comment 27

15 years ago
*** Bug 186415 has been marked as a duplicate of this bug. ***

Comment 28

15 years ago
Created attachment 110378 [details] [diff] [review]
Patch used in chimera to hook  up Copy Image

This patch allows chimera to implemenet Copy Image in a context menu. Hope it
will be useful here.

Note that it copies a chunk of code from Content (nsContentAreaDragDrop.cpp) to
layout (nsCopySupport). The copy/drag and drop code really needs major
refactoring to avoid duplicate code.

Comment 29

15 years ago
Over to Shuehan.
Assignee: blaker → shliang

Comment 30

15 years ago
*** Bug 187961 has been marked as a duplicate of this bug. ***

Comment 31

15 years ago
Bug 193053 has patches to add Image copy, and drag and drop of images.
(Reporter)

Updated

15 years ago
Depends on: 193053

Comment 32

15 years ago
*** Bug 193392 has been marked as a duplicate of this bug. ***

Updated

15 years ago
Target Milestone: --- → mozilla1.4beta

Comment 33

14 years ago
See also bug 210043, same bug for Firebird.

Comment 34

14 years ago
Is this scheduled to be fixed for Moz 1.4 final? This is a hugely important bug
in my book, as I need to be able to copy images to Photoshop.

Comment 35

14 years ago
As I said in another bug (which was a duplicate of this), this bug is the only 
reason why I still use Internet Explorer. I need to be able to copy images to 
work on my websites. They have known about this bug for some time and seem to 
refuse to even consider fixing it. I doubt it would take them long to add this 
feature to a new version of Mozilla.

Comment 36

14 years ago
To the comments that copy image to clipboard should replace copy link location: the link location is quite useful when you're on a bulletin board and need to paste in the location so that others can see your link. It's also useful if you want to inline the image into a bulletin board post.

I have a workaround for this problem that I use now though I'm not a heavy image user. ContextMenu extensions provides enough tools so that you can piece together a script to do the work for you. I have a context menu option to copy an image to the clipboard and another to drop the image into MS Paint. It's klunky but it works.

Comment 37

14 years ago
I said that "Copy Image" should replace "Copy Image Location", not "Copy Link
Location".  You would still be able to get an image's URL using "View Image" or
"Properties".  Also, pasting an image copied by "Copy Image" into something that
only accepts text might paste the URL.
Yes, Copy Image should replace Copy Image Location, and should copy the URI in
the text format, and the image in the bitmap format. (Clipboards can store more
than one format.)

Note, by the way, for those of you who must have this feature to use Mozilla,
that you _can_ just _drag_ the image you want to your paint program.

Comment 39

14 years ago
To copy an image into Photoshop 7, you have to drag the image to Photoshop's
window in the taskbar, then wait for Photoshop to pop up, then find the image
you want to paste it into (you can't paste it as a new image) and drop it. This
is a terrible workaround.

I would like to see "Copy Image" work exactly as it does in IE6. I don't even
think it's necessary to copy the image location as text as well, as that may
have potential to screw up other apps that are looking for an image only. "Copy
Image" should do exactly as stated -- copy the image to the clipboard. You can
always "View Image" and copy the URL that appears in the URL bar if you want the
image location, or right-click -> Properties -> copy and paste the location that
appears. People are used to using both of these behaviors from IE.

Please implement this feature, and implement it so it just copies the image. It
doesn't need to be any fancier than that. (I don't want to have to keep
reporting bugs if it ends up that Photoshop or some other program won't accept
an image on the clipboard if there is text there as well.)
Multiple clipboard formats are quite well supported, that's not a problem.

Comment 41

14 years ago
Comment on attachment 110378 [details] [diff] [review]
Patch used in chimera to hook  up Copy Image

This patch is on the trunk now.
Attachment #110378 - Attachment is obsolete: true

Comment 42

14 years ago
Attachment 114287 [details] [diff] in bug 193053 is a patch to add Copy Image to the image
context menu. As well as that, I believe some work needs to be done in Windows
widget code to put image data on the native clipboard.

Comment 43

14 years ago
erica: speak for yourself about being used to behaviors from IE.  I personally
am not used to any behaviors from IE and don't want it to become harder for me
to get an image URL just because IE makes it hard for a user to get an image URL.

Comment 44

14 years ago
Maybe it's because I'm not a Mozilla programmer, but I really don't understand
the problem. I can't imagine it can be that difficult to implement *both* a
"Copy image" and a "Copy image location" function, in two separate menu items. I
don't see why these two should exclude or conflict with each other.

It's really not about IE compatibility or something, it's just a feature that
some of us are desperately missing.

Comment 45

14 years ago
"I need to be able to copy images to work on my websites."

I'm ©urious: What kind of fair use do you make of such images?

"you _can_ just _drag_ the image you want to your paint program."

Not to GIMP for Windows.  I can drag files from Windows Explorer to GIMP,
but when I try to drag an image from Mozilla to GIMP, all I get is the
"no" cursor.  Want a test case?

"I can't imagine it can be that difficult to implement *both* a
'Copy image' and a 'Copy image location' function"

The UI designers try to eliminate unnecessary items from context menus.

Comment 46

14 years ago
Excuse-me, I sent the message below for Bug 21747, I wanted to send it for Bug
135300. Sorry for that... So I paste my message here :

Yes I think that "Copy Image" is a very useful feature that lacks in Mozilla 1.4
. I think it would be great if there were "Copy Image" and "Copy Image Location"
(in order to have a short context menu, why not doing a submenu called "Menu"
and when you move the mouse over it, it appears two items at the right of Menu
with the two items mentioned above ?), even if I don't use Copy Image Location.
In Opera (7.11), there are the two options in the context menu.
A context menu with a lot of items would not somehow be very annoying.
> (Clipboards can store more than one format.)

Is this a safe assumption on all platforms?  It would be bad to break 
platform parity over a trivial thing like combining two menu items.

Comment 48

14 years ago
I just want to add that in my last message concerning this bug (#46), I wanted
to say that the menu could be called "Copy" (and not "Menu"). In it, there could
be "Copy image location", "Copy Image" and, if the image is a link "Copy link
location" (it would thus be a menu like File > New in Mozilla 1.4).

Comment 49

14 years ago
Created attachment 127574 [details] [diff] [review]
updated version of context menu patch from bug 193053

updated version of attachment #114287 [details] [diff] [review] for adding context menu item, also move
Copy Image to just above Copy Image Location.
I think this is all that is needed, for windows as well as mac.

Comment 50

14 years ago
i've been annoyed at the lack of a "right click -> copy image" since i first
starting using this browser.

i vote strongly in favor of adding it as the original bug states and not using
any the half-backward workarounds suggested (for example, by jesse) that do not
fix the problem.

Updated

14 years ago
Target Milestone: mozilla1.4beta → ---

Comment 51

14 years ago
I don't like it when they remove "Copy Image Location". Why not add a new menu
item? What if people need to copy the location of that image? Install yet
another extension? mozilla is more and more becomming 'puzzleware'. Great for
new users. Glue your own extensions to make a perfectly working/functional MSIE
clone...
I would like to strongly support this bug. The user interface *is* the program.
If users can't see it, they don't know about it. I'm a programmer, and I thought
it was odd, but I assumed it was just something that could have been difficult
on one platform and so had been conveniently ignored.

Windows users in particular are used to being able to right-click on things to
copy them, in just about every program that offers some form of clipboard
facility. If they don't see it there, they'll just assume that Mozilla/Firebird
doesn't support it. They won't go out and get it as an extension - none of my
artist friends would even think of that, let alone be interested in the hassle.
They'll use IE, a substandard browser, for the sake of a context menu item.

Comment 53

13 years ago
(In reply to comment #52)
> Windows users in particular are used to being able to right-click on things to
> copy them, in just about every program that offers some form of clipboard
> facility. If they don't see it there, they'll just assume that Mozilla/Firebird
> doesn't support it. They won't go out and get it as an extension - none of my
> artist friends would even think of that, let alone be interested in the hassle.
> They'll use IE, a substandard browser, for the sake of a context menu item.

Wow you just described me perfectly!
So our group writes the code for all of our web based apps and I end up doing
all the user documentaion.  For most of that user documentation. I basically
take screen shots with alt-prtscn and paste and crop into word or powerpoint. If
i only want a single pic from a page right clicking and pasting is such a no
brainer that I can't live without it.  Just thought you might like to know why i
still cant use Mozilla for day today use.

Comment 54

13 years ago
It´s necessry to simplify the right-click menu and it will be more intuitive.
I suggest:
  > Copy
  > Copy Location 

Copy: simple copy (cut-paste) for any object: Image, Text, ...
Copy Location: active for objects with Location.

As the option "Show Image" I don´t understand its mission: a simple view in a
new window!! It´s very obvious, the image it´s just been showed.

Comment 55

13 years ago
*** Bug 242214 has been marked as a duplicate of this bug. ***

Comment 56

13 years ago
In regards to Comment #54, I agree that "Copy" would be more intuitive.  On a
longer context menu, having the shorter item "copy" instead of "copy image"
would allow the brain to distinguish the item more easily.

Any possibility of getting it shortened to just "copy"?

Comment 57

13 years ago
there are correct fixes for this ui, but they don't include as many menu items
as we have. (they still require supporting the feature)
(which isn't implemented... [at least for some platforms])
<ben> there's no way two copy menu items are needed for images. both data
flavors should be added to the transferable in a single "Copy" item.

<ben> plain text applications can get the text version, rich text apps can get
+the image version, or something else through "Edit->Paste Special..."
(see MS Word and other well behaved applications)

it's been around forever; it's the right thing to do
<bz> it'd simplify the "link location / image location" **** I always get with
+linked images...

--
This is a summary of sorts of an irc conversation. Ben and I agree completely
here. ben was nice enough to do the writing.

neil: would you please sign off on changing the spec?
Assignee: shliang → guifeatures
QA Contact: bugzilla

Comment 58

13 years ago
Only btw: This extension works for me with Seamonkey (Windows).
http://extensionroom.mozdev.org/more-info/copyimage

Comment 59

13 years ago
timeless, looks like Ian beat ben to it by almost two years (comment 15 :-)

Summary for the volunteer:
* Fix nsCopySupport.cpp to copy the location and image together
* Change the menuitem to copy image
(Separate patches accepted, in order!)
Keywords: helpwanted
*** Bug 242739 has been marked as a duplicate of this bug. ***

Comment 61

13 years ago
Is there any work being done on porting FireFox's funcitonality from bug 210043
to Seamonkey?

Comment 62

13 years ago
No, because it doesn't implement comment 57.
Product: Core → Mozilla Application Suite

Comment 63

13 years ago
Isn't this bug fixed now?  I've got a Copy Image item on my context menu in
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041117
MultiZilla/1.7.0.0h and have had for some time now.
(Assignee)

Comment 64

13 years ago
Created attachment 169824 [details] [diff] [review]
non-UI patch v1

In order.
comment #59

> * Fix nsCopySupport.cpp to copy 
> the location and image together
(Assignee)

Updated

13 years ago
Attachment #169824 - Flags: review?(bzbarsky)
Comment on attachment 169824 [details] [diff] [review]
non-UI patch v1

First off, using the -p context to diff and more context, say "diff -up8",
makes diffs easier to read.

>Index: mozilla/content/base/src/nsCopySupport.cpp
>+  //Add the unicode data flavor

This code largely duplicates the code in DocumentViewerImpl::CopyImageLocation
(which seems to be the current implementation of the "copy image location"
method).  I'd prefer the two methods share code (both call the same static
method in nsCopySupport or nsContentUtils or something).

More seriously, the code here puts the image only in aClipboardID (which you're
passing as nsIClipboard::kGlobalClipboard, whereas the existing "copy image
location" code puts the URL in both nsIClipboard::kGlobalClipboard and
nsIClipboard::kSelectionClipboard.  I don't think we really want to break
that....
Attachment #169824 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 66

13 years ago
Created attachment 169902 [details] [diff] [review]
non-UI patch v2

Thanks the review.
Addressed those 2 issues...

Currently, copyImageContents doen't support unix,
*so* copyImageContents is using only the global
clipboard, right? Or is there any problem to set
raw image flavor onto the linux selection clipboard?
Attachment #169824 - Attachment is obsolete: true
Attachment #169902 - Flags: review?(bzbarsky)
Comment on attachment 169902 [details] [diff] [review]
non-UI patch v2

>Index: mozilla/content/base/public/nsCopySupport.h
>+    static nsresult ImageCopy(nsIImageLoadingContent* aImageElement,
>+                              PRBool aContent);

I'd call that last arg "aCopyImageData".

>Index: mozilla/content/base/src/nsCopySupport.cpp
>+  nsCOMPtr<nsITransferable> trans(do_CreateInstance(kCTransferableCID));
>+  NS_ENSURE_TRUE(trans, NS_ERROR_FAILURE);

Why suppress the actual rv from do_CreateInstance here?

>+  nsCOMPtr<nsISupportsString>
>+     location(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID));
>+  NS_ENSURE_TRUE(location, NS_ERROR_FAILURE);

Same here.

>+  if(aContentsFlag){

Spaces after "if" and before '{', please.

>+    // get the image data form the element

s/form/from/

>+    nsCOMPtr<nsISupportsInterfacePointer>
>+      imgPtr(do_CreateInstance(NS_SUPPORTS_INTERFACE_POINTER_CONTRACTID));
>+    NS_ENSURE_TRUE(imgPtr, NS_ERROR_FAILURE);

Again, why suppress return value?

>+  // get clipboard
>+  nsCOMPtr<nsIClipboard> clipboard(do_GetService(kCClipboardCID));
>+  NS_ENSURE_TRUE(clipboard, NS_ERROR_FAILURE);

And here.

I don't really follow comment 66; we're putting the transferable with the image
contents on both global and selection clipboard; I suspect the image contents
data flavor will just get ignored on the latter....  The non-support on Unix is
not in this function but in the widget code that handles getting the image data
out of the transferable if someone asks for it.  So the patch looks correct in
general except for the above nits.
Attachment #169902 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 68

13 years ago
Created attachment 169960 [details] [diff] [review]
non-UI patch v3

The difference is based on comment #67 .
Attachment #169902 - Attachment is obsolete: true
Attachment #169960 - Flags: review?(bzbarsky)
Comment on attachment 169960 [details] [diff] [review]
non-UI patch v3

Actually, do_CreateInstance guarantees an error rv on null return.  So no need
for those NS_ENSURE_TRUE checks....

r=bzbarsky with that.  jst, could you sr?
Attachment #169960 - Flags: superreview?(jst)
Attachment #169960 - Flags: review?(bzbarsky)
Attachment #169960 - Flags: review+
(Assignee)

Comment 70

13 years ago
Created attachment 169964 [details] [diff] [review]
non-UI patch v4

Removed null checks as comment #69 .
Attachment #169960 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #169960 - Flags: superreview?(jst)
(Assignee)

Updated

13 years ago
Attachment #169964 - Flags: superreview?(jst)
Attachment #169964 - Flags: review?(bzbarsky)
(Assignee)

Comment 71

13 years ago
Created attachment 169972 [details] [diff] [review]
FE patch v1 (Navigator and Mail/News)

This patch corresponds to 
> * Change the menuitem to copy image
at the comment #59

In the long run, maybe it's time to delete
<command id="cmd_copyImageLocation"/>
elements in various files,
http://lxr.mozilla.org/mozilla/search?string=id%3D%22cmd_copyImageLocation%22
because nobody !(seamonkey, fx, tb, camino?) use
this command.

On the other hand,
goDoCommand("cmd_copyImageLocation")
still just works, and they doesn't hurt anybody,
other than the "Avoid Possible Bloat(tm)" strategy.

IMHO, they should be removed especially from
browser and mail components, but I'd like to
wait-and-see here.
Attachment #169972 - Flags: review?(dean_tessman)
Comment on attachment 169964 [details] [diff] [review]
non-UI patch v4

r=bzbarsky
Attachment #169964 - Flags: review?(bzbarsky) → review+

Comment 73

13 years ago
Re comment #71:
> In the long run, maybe it's time to delete
> <command id="cmd_copyImageLocation"/> elements in various files, [...]
> because nobody !(seamonkey, fx, tb, camino?) use this command.
> 
> On the other hand, goDoCommand("cmd_copyImageLocation")
> still just works, and they doesn't hurt anybody,

Yes, and it's still useful and even "revived" by certain addons.

> other than the "Avoid Possible Bloat(tm)" strategy.

I don't think that the impact is high enough to justify a removal.

Comment 74

13 years ago
cmd_CopyImageContents? :-/
(Assignee)

Comment 75

13 years ago
(In reply to comment #74)
> :-/

You mean we should use "cmd_copyImage" instead?
(Assignee)

Updated

13 years ago
Attachment #169964 - Attachment is obsolete: true
Attachment #169964 - Flags: superreview?(jst)
(Assignee)

Updated

13 years ago
Attachment #169972 - Attachment is obsolete: true
Attachment #169972 - Flags: review?(dean_tessman)
(Assignee)

Comment 76

13 years ago
Created attachment 170021 [details] [diff] [review]
non-UI patch v5

I pondered for a while, and I'd like to suggest
some interface changes.

nsIContentViewerEdit:
1. Add "void copyImage();" to this interface.
2. copyImageLocation() ,copyImageContents() and 
copyImage() have very the same functionality.
Copy both location and image data to the clipboard.
3. Remove copyImageLocation() and 
copyImageContents() in the future.
Maybe Mozilla 1.8 or 2.0 or 3.0 
4. Get ready to remove them.

webshell:
1. Support the command "cmd_copyImage" as well as
"cmd_copyImageLocation" and "cmd_copyImageContents"
2. These three commands works identically.
Copy both location and image data to the clipboard.
3. cmd_copyImageLocation and cmd_copyImageContents
are deprecated. To be removed as well.

Boris, would you review this idea?
(Assignee)

Updated

13 years ago
Attachment #170021 - Flags: review?(bzbarsky)

Comment 77

13 years ago
Created attachment 170024 [details] [diff] [review]
cmd_copyImageLocation is unnecessary

This is the basics of how to remove cmd_copyImageLocation.
(Assignee)

Comment 78

13 years ago
Comment on attachment 170021 [details] [diff] [review]
non-UI patch v5

Sorry for bug spam.
nsIClipboardCommands is @status FROZEN
Attachment #170021 - Attachment is obsolete: true
Attachment #170021 - Flags: review?(bzbarsky)
(Assignee)

Updated

13 years ago
Attachment #169972 - Attachment is obsolete: false
Attachment #169972 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

13 years ago
Attachment #169964 - Attachment is obsolete: false
Attachment #169964 - Flags: superreview?(jst)

Comment 79

13 years ago
Comment on attachment 169972 [details] [diff] [review]
FE patch v1 (Navigator and Mail/News)

r=me once the copied image pastes into Notepad, Paint, NVu etc.
Attachment #169972 - Flags: review?(neil.parkwaycc.co.uk) → review+
Comment on attachment 169964 [details] [diff] [review]
non-UI patch v4

sr=jst
Attachment #169964 - Flags: superreview?(jst) → superreview+
Assignee: guifeatures → torisugari
Comment on attachment 169964 [details] [diff] [review]
non-UI patch v4

I just checked in this patch for 1.8b
(Assignee)

Comment 82

13 years ago
(In reply to comment #81)
> (From update of attachment 169964 [details] [diff] [review] [edit])
> I just checked in this patch for 1.8b

bz, thank you.
Then I should check in attachment 169964 [details] [diff] [review] for 1.8b,
as well. 

That does not mean that this bug is going to be
marked "resolved" for 1.8b, because Neil and other
guys' (ben, bz, and timeless in comment #57)
request is cmd_copyImage[Contents] should copy
also the HTML fragment. I think there shold be a
sticky way to create the transferable per
element/selection.

I'm planning to move the class
nsTransferableFactory
from nsContentAreaDragDrop.cpp
to nsCopySupport.h/.cpp
(Assignee)

Comment 83

13 years ago
(In reply to comment #82)
> Then I should check in attachment 169964 [details] [diff] [review] [edit] 
Oops, I mean comment 169972 the front end patch.
Status: NEW → ASSIGNED
That patch doesn't have sr yet, does it?
(Assignee)

Updated

13 years ago
Attachment #169972 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 85

13 years ago
Comment on attachment 169972 [details] [diff] [review]
FE patch v1 (Navigator and Mail/News)

You're working on copy image from navigator, paste into editor I hope?
Attachment #169972 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Comment on attachment 169972 [details] [diff] [review]
FE patch v1 (Navigator and Mail/News)

Checked in for 1.8b
(Assignee)

Comment 87

13 years ago
Created attachment 171394 [details] [diff] [review]
non-UI patch v6

(In reply to comment #85)
> You're working on copy image from navigator, >paste into editor I hope?

This patch is to make it possible to paste to
composer, though this is not so ambitious as I said
before.

> I'm planning to move the class
> nsTransferableFactory
> from nsContentAreaDragDrop.cpp
> to nsCopySupport.h/.cpp

bz, thank you for checking in, again.
Attachment #171394 - Flags: review?(bzbarsky)
Comment on attachment 171394 [details] [diff] [review]
non-UI patch v6

>Index: mozilla/content/base/public/nsCopySupport.h
>+  private:
>+    // copy string data onto the transferable
>+    static nsresult AppendString(nsITransferable *aTransferable,
>+                                 const nsAString& aString,
>+                                 const char* aFlavor);
>+
>+    // copy HTML node data
>+    static nsresult AppendDOMNode(nsITransferable *aTransferable,
>+                                  nsIDOMNode *aDOMNode);

Why not just make these non-class statics in nsCopySupport.cpp?

>Index: mozilla/layout/base/nsDocumentViewer.cpp

> NS_IMETHODIMP DocumentViewerImpl::CopyImageLocation()

Why the changes here?  I'd prefer that we catch non-image nodes here, and QI to
nsIDOMNode in nsCopySupport methods if needed...

Comment 89

13 years ago
(In reply to comment #88)
>>Index: mozilla/layout/base/nsDocumentViewer.cpp
>>NS_IMETHODIMP DocumentViewerImpl::CopyImageLocation()
>Why the changes here?  I'd prefer that we catch non-image nodes here, and QI to
>nsIDOMNode in nsCopySupport methods if needed...
Because GetPopupImageNode works by getting a DOM Node and calling QI to
nsImageLoadingContent; now that CopyImageLocation needs a DOM Node there's
little point calling GetPopupImageNode, because you'll only have to QI back and
forth. Note that this leaves IsInImage as the last caller of GetPopupImageNode.
(Assignee)

Comment 90

13 years ago
(In reply to comment #88)
> Why the changes here?  I'd prefer that we catch non-image
> nodes here, and QI to nsIDOMNode in nsCopySupport
> methods if needed...

As already stated in comment #89, it simply reduces the
total number of QIs.
(Assignee)

Comment 91

13 years ago
Created attachment 171485 [details] [diff] [review]
non-UI patch v7

along with comment #88
(Assignee)

Updated

13 years ago
Attachment #171394 - Attachment is obsolete: true
Attachment #171485 - Flags: review?(bzbarsky)
(Assignee)

Updated

13 years ago
Attachment #171394 - Flags: review?(bzbarsky)

Comment 92

13 years ago
Sorry, I know this is off-topic, but why do I keep getting posts on this bug by
E-mail, even though I removed my address (bugzilla@sunlight.de) from the CC
list? Can someone give me a hint?

Comment 93

13 years ago
(In reply to comment #92)
> Sorry, I know this is off-topic, but why do I keep getting posts on this bug by
> E-mail, even though I removed my address (bugzilla@sunlight.de) from the CC
> list? Can someone give me a hint?

you have voted for this bug so either remove your vote or change your email
settings in preferences
Comment on attachment 171485 [details] [diff] [review]
non-UI patch v7

It'll be a few days before I can get to this.

Comment 95

13 years ago
Patches here are breaking Copy Image in Camino.

On Mac we don't necessarily want 'Copy Image' to put both the URL and the image
data onto the clipboard, because some apps will take the text data (the URL)
first, when the user really wants to paste the image data.

I think the embedder/caller needs to be able to specify what 'copy image' really
means.
Hmm.. That causes some issues with comments from Marlon and Ben (via timeless)
earlier in this bug.  I think they were assuming that apps would do the Right
Thing with multiple clipboard flavors, and it sounds like they don't....

So before we patch more stuff, could we clearly spec out the api we want?

Comment 97

13 years ago
I think the underlying layers (nsCopySupport) needs to be told what to put in
the clipboard:

nsresult ImageCopy(nsIImageLoadingContent* aImageElement, PRBool aCopyImageData,
PRBool aCopyImageLocation);

Maybe we can then use nsICommandParams to feed down flags from the UI layer
about what the UI wants copied.
Sure.  That seems reasonable.
or maybe an PRInt32 aCopyFlags arg? with COPY_IMAGE and COPY_TEXT constants,
that can be ORd together.

Comment 100

13 years ago
Is it a good idea to even copy text data along with the image data?

One thing about trying to paste an image inside a program that doesn't support
images, is that it will not allow it -- thus indicating to the user that the
program does not support pasting the data they requested.  Pasting text instead
of the requested image might not be desirable.
Kevin, please read earlier discussion in this bug.  Apparently at least some
Gecko-based applications (Firefox, eg) believe that this is desirable, yes.
(In reply to comment #99)
>COPY_IMAGE and COPY_TEXT
Not forgetting COPY_HTML!
winword.exe also prefers text to bitmap, although I would have thought it would
prefer HTML to text given the choice.
(Assignee)

Comment 104

13 years ago
(In reply to comment #95)
> some apps will take the text data (the URL)
> first, when the user really wants to paste the
> image data.

Then what happens on Drag and Drop?

> I think the embedder/caller needs to be able
> to specify what 'copy image' really means.

Hmm, I guess this is not a embedding issue,
because all the Mac products would be affected.
Though I agree to making the api more flexible
as suggested in comment #97, comment #99 and
comment #102. 

IMHO, the difficulty lies in FE rather than api.
The front end is going to be painfully complicated,
as it's no more "XP"FE ...

Neil:
Is it ok to put #ifdef XP_MAC on a xul file
a la firefox? Or I should use
platformCommunicatorOverlay.xul
instead?

I sometimes think we should give up enhancing
copyImageContents and just show "Copy" menuitem
even if nothing is selected (In this case, copy
the popupNode instead of the selection). 
By the way, is there any way we can affect which flavor on the clipboard is the
"primary" one?  Or is it completely up to the app being pasted into to decide
what it wants?

Comment 106

13 years ago
Applications usually look in the clipboard for flavors in a fixed order,
normally richest (rich text) to simplest (plain text).

The problem with images and text is that they are such different flavors; they
are not variations of the same flavor. Inserting an image is a very different
user action than inserting text, and most users will make a conscious choice to
get one or the other. Hence the need to keep control over which we put in the
clipboard.
what simon said. the application is in control of the order in which it
searches. it's out of our hands entirely.

Comment 108

13 years ago
(In reply to comment #107)
> what simon said. the application is in control of the order in which it
> searches. it's out of our hands entirely.
Can we have an option to make this feature only copy image data (and no text)?

Kevin, please please please read the bug like I asked you to.  Please.  It'll
really help, or so I hope.
(Assignee)

Updated

13 years ago
Attachment #171485 - Flags: review?(bzbarsky)
(Assignee)

Comment 110

13 years ago
Created attachment 172421 [details] [diff] [review]
non-UI patch v8

* flags (comment #99)
* leaving out Mac
> +#if defined(XP_MAC) || defined(XP_MACOSX)
Attachment #171485 - Attachment is obsolete: true
Attachment #172421 - Flags: review?(bzbarsky)
(Assignee)

Comment 111

13 years ago
(In reply to comment #106)
> Hence the need to keep control over which we put
> in the clipboard.

To "keep control", we need one more "copy"
menuitem to implement the HTML thingy.

-------------------
View Image
Block Image from...
Copy Image Location #URL only
Copy Image Contents #Image data only
-------------------
Copy                #Sink: URL, image and HTML
-------------------

?
Comment on attachment 172421 [details] [diff] [review]
non-UI patch v8

-	 shortcut.Append(PRUnichar('\n'));
+	 shortcut.AppendLiteral("\n");

why this change?


Also, do we really want this to be different on different platforms?

Comment 113

13 years ago
Comment on attachment 172421 [details] [diff] [review]
non-UI patch v8

> NS_IMETHODIMP DocumentViewerImpl::CopyImageContents()
> {
>   NS_ENSURE_TRUE(mPresShell, NS_ERROR_NOT_INITIALIZED);
>   nsCOMPtr<nsIImageLoadingContent> node;
>   GetPopupImageNode(getter_AddRefs(node));
>   // make noise if we're not in an image
>   NS_ENSURE_TRUE(node, NS_ERROR_FAILURE);
> 
>-  return nsCopySupport::ImageCopy(node, PR_TRUE);
>+#if defined(XP_MAC) || defined(XP_MACOSX)
>+  return nsCopySupport::ImageCopy(node, COPY_SUPPORT_IMAGE);
>+#else
>+  return nsCopySupport::ImageCopy(node, COPY_SUPPORT_TEXT |
>+                                        COPY_SUPPORT_HTML |
>+                                        COPY_SUPPORT_IMAGE);
>+#endif
> }

I don't think this is the right thing to do. I think you need to feed the flags
down from the UI layer via nsICommandParams. A Mac embedder *might* want all
the flavors; let the embedder decide, not gecko.
Comment on attachment 172421 [details] [diff] [review]
non-UI patch v8

Yes, what Simon said.  We need to expose an api that will let the embeddor
choose the sort of copy that happens.  This code should have no ifdefs.  What
it should have is a document viewer api that takes flags, probably, and this
change should be propagated all the way to whatever the embedding interface is
that is calling into this code.
Attachment #172421 - Flags: review?(bzbarsky) → review-

Comment 115

13 years ago
Canm we get get some traction on this? It's blocking Camino 0.9.
If we can't get traction on the more flexible back end for 1.8b, I think we
should back the earlier patches out....  In any case, one or the other should
definitely block 1.8b2.
Flags: blocking1.8b2?

Updated

12 years ago
Blocks: 279270

Comment 117

12 years ago
We're not getting any traction on this. Can we back this out? Camino is still
broken.
Created attachment 176516 [details] [diff] [review]
Backout of those patches in this bug that have reviews (and hence presumably landed)

Comment 119

12 years ago
*** Bug 285317 has been marked as a duplicate of this bug. ***
Attachment #176516 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176516 - Flags: review?(torisugari)
Created attachment 176909 [details] [diff] [review]
Move the flags to nsIContentViewerEdit

bz, is this what you mean?
Attachment #176909 - Flags: review?(bzbarsky)

Comment 121

12 years ago
Comment on attachment 176909 [details] [diff] [review]
Move the flags to nsIContentViewerEdit

>Index: docshell/base/nsIContentViewerEdit.idl
>===================================================================
>RCS file: /cvsroot/mozilla/docshell/base/nsIContentViewerEdit.idl,v
>retrieving revision 1.5
>diff -p -u -d -r1.5 nsIContentViewerEdit.idl
>--- docshell/base/nsIContentViewerEdit.idl	17 Apr 2004 21:49:36 -0000	1.5
>+++ docshell/base/nsIContentViewerEdit.idl	9 Mar 2005 17:54:45 -0000
>@@ -40,7 +40,7 @@
> 
> #include "nsISupports.idl"
> 
>-[scriptable, uuid(42d5215c-9bc7-11d3-bccc-0060b0fc76bd)]
>+[scriptable, uuid(1691a02f-53b2-4cb8-8769-48e7efc908b8)]
> interface nsIContentViewerEdit : nsISupports
> {
> 	void search();
>@@ -55,8 +55,11 @@ interface nsIContentViewerEdit : nsISupp
> 	void copyLinkLocation();
> 	readonly attribute boolean inLink;
> 
>-	void copyImageLocation();
>-	void copyImageContents();
>+	const long COPY_TEXT = 0x0001;
>+	const long COPY_HTML = 0x0002;
>+	const long COPY_DATA = 0x0004;
>+	const long COPY_ALL = -1;

I'd prefer these flags be called COPY_IMAGE_FOO, unless there's
a plan to use them elsewhere.

>Index: dom/src/base/nsGlobalWindowCommands.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindowCommands.cpp,v
>retrieving revision 1.15
>diff -p -u -d -r1.15 nsGlobalWindowCommands.cpp
>--- dom/src/base/nsGlobalWindowCommands.cpp	18 Feb 2005 07:29:40 -0000	1.15
>+++ dom/src/base/nsGlobalWindowCommands.cpp	9 Mar 2005 17:54:46 -0000
>@@ -603,9 +603,9 @@ nsresult
> nsClipboardImageCommands::DoClipboardCommand(const char *aCommandName, nsIContentViewerEdit* aEdit, nsICommandParams* aParams)
> {
>   if (!nsCRT::strcmp(sCopyImageLocationString, aCommandName))
>-    return aEdit->CopyImageLocation();
>+    return aEdit->CopyImage(nsIContentViewerEdit::COPY_TEXT);
> 
>-  return aEdit->CopyImageContents();
>+  return aEdit->CopyImage(nsIContentViewerEdit::COPY_ALL);

This will still break Mac. CopyImage() needs to use COPY_DATA,
or you need to pass the flags down in aParams.
Created attachment 176945 [details] [diff] [review]
Provide the possibility of doCommandParams
Attachment #176909 - Attachment is obsolete: true
Attachment #176945 - Flags: review?(bzbarsky)
Attachment #176909 - Flags: review?(bzbarsky)
I'll try to look tomorrow, but why is this stuff on nsIContentViewerEdit anyway?
 Copying is not an editing-related thing, really (though pasting is).

Comment 124

12 years ago
nsIContentViewerEdit is an API created before nsICommands became the One True
Way to do stuff like this (and they aren't used everywhere either).
Ah, ok.  So this is an api we should be striving to deprecate and remove, then?

Comment 126

12 years ago
I think nsIContentViewFile and nsIContentViewEdit should go away, yes. 
Comment on attachment 176945 [details] [diff] [review]
Provide the possibility of doCommandParams

>Index: content/base/src/nsCopySupport.cpp
>+static nsresult AppendString(nsITransferable 
>+  return aTransferable->SetTransferData(aFlavor, data,
>+                                        (aString.Length() * 2));

I know that's what all this code had, but how about s/2/sizeof(PRUnichar)/ ?

With that, r=bzbarsky.

Simon, can you sr?
Attachment #176945 - Flags: superreview?(sfraser_bugs)
Attachment #176945 - Flags: review?(bzbarsky)
Attachment #176945 - Flags: review+

Updated

12 years ago
Attachment #176945 - Flags: superreview?(sfraser_bugs) → superreview+
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Depends on: 210043
Flags: blocking1.8b2?
So, have you filed a bug about either remvoing the Copy Image Location command
or changing the Copy Image command params in Firefox's frontend? Firefox has
both commands in its UI. The last includes the former....

Comment 130

12 years ago
(In reply to comment #129)
> So, have you filed a bug about either remvoing the Copy Image Location command
> or changing the Copy Image command params in Firefox's frontend? Firefox has
> both commands in its UI. The last includes the former....
> 
If you are referring to just copying image data in Firefox, then that particular
"bug" has been filed here: Bug 286118

Updated

12 years ago
Attachment #176516 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176516 - Flags: review?(torisugari)

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.