Bug 195691 - allow users to mark up screenshots
Summary: allow users to mark up screenshots
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 2.3   Edit
Assignee: Willian Mitsuda CLA
QA Contact:
URL:
Whiteboard:
Keywords: bugday, helpwanted
Depends on:
Blocks:
 
Reported: 2007-07-06 14:47 EDT by Mik Kersten CLA
Modified: 2008-03-14 22:10 EDT (History)
1 user (show)

See Also:


Attachments
Proposed patch (23.30 KB, patch)
2007-11-17 15:55 EST, Willian Mitsuda CLA
no flags Details | Diff
mylyn/context/zip (18.93 KB, application/octet-stream)
2007-11-17 15:55 EST, Willian Mitsuda CLA
no flags Details
example (224.09 KB, image/png)
2007-11-17 16:44 EST, Eugene Kuleshov CLA
no flags Details
Alternative patch (27.65 KB, patch)
2007-11-18 00:42 EST, Willian Mitsuda CLA
no flags Details | Diff
mylyn/context/zip (19.57 KB, application/octet-stream)
2007-11-18 00:42 EST, Willian Mitsuda CLA
no flags Details
Screenshot (66.96 KB, image/jpeg)
2008-02-04 22:05 EST, Willian Mitsuda CLA
no flags Details
Another UI prototype screenshot (70.15 KB, image/jpeg)
2008-02-04 22:32 EST, Willian Mitsuda CLA
no flags Details
Another toolbar prototype: text under icons (63.35 KB, image/jpeg)
2008-02-04 22:36 EST, Willian Mitsuda CLA
no flags Details
Patch (50.90 KB, patch)
2008-02-20 13:42 EST, Willian Mitsuda CLA
no flags Details | Diff
mylyn/context/zip (281.30 KB, application/octet-stream)
2008-02-20 13:43 EST, Willian Mitsuda CLA
no flags Details
Patch for color selection UI (8.09 KB, patch)
2008-02-21 00:32 EST, Willian Mitsuda CLA
no flags Details | Diff
Patch for mark up functionality (32.11 KB, patch)
2008-02-21 00:35 EST, Willian Mitsuda CLA
no flags Details | Diff
ms word color picker and crop icon (8.04 KB, image/png)
2008-02-22 05:21 EST, Eugene Kuleshov CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mik Kersten CLA 2007-07-06 14:47:44 EDT
When attaching screenshots to bugs it would be very useful to allow users to mark up the screenshot with a pen-like tool that allows the to circle or highlight items.  Optionally we could also provide them with a text tool, but a pen tool should be sufficient.
Comment 1 Mik Kersten CLA 2007-10-09 00:29:09 EDT
Good bugday candidate for anyone with good image manipulation/drawing skills.
Comment 2 Mik Kersten CLA 2007-11-16 01:28:08 EST
I wonder if a text or callout tool would actually be more useful than a pen tool.  I liked marking up screenshots by hand back when I used a tablet PC, but now find myself marking them up with text more frequently.  
Comment 3 Willian Mitsuda CLA 2007-11-16 13:32:05 EST
I can take this one.

(In reply to comment #2)
> I wonder if a text or callout tool would actually be more useful than a pen
> tool.  I liked marking up screenshots by hand back when I used a tablet PC, but
> now find myself marking them up with text more frequently.  
> 

I think simply mark by drawing is important, let's say you want to spot some specific area in screenshot. It is useful to mark pixel alignment bugs, for example.

Be able to write some text is interesting too, although I haven't used it so much.

Since writing text would involve a little more work, I think it would be better to open another bug to address this separately.
Comment 4 Eugene Kuleshov CLA 2007-11-16 14:11:32 EST
I have some concerns about these image editing features. Basically it is either implementing not enough or too much. So, here are few random thoughts:

-- what about color selection and line width for this drawing tool? should this editor allow to select those?

-- complicated image editing features will be hard to support on all platforms

-- editing should provide undo/redo buffer to be really practical, i.e. allow to add image markup, then edit attachment text, then make updates/changes to the markup, etc.

-- also, using a modal attachment dialog as a complicated image editor doesn't really fit well into the workbench UI. Basically user is forced to spend significant time in the modal dialog before he can do anything else in workbench, because of to the way this wizard is currently implemented user either have to go all the way trough and submit attachment or drop his changes in the middle and start over later
Comment 5 Willian Mitsuda CLA 2007-11-17 15:55:53 EST
Created attachment 83155 [details]
Proposed patch

First proposal:

- Implemented a basic "mark" tool.
- You can either select "Crop" (default) or "Mark". When marking, the selection (if any) will be dashed, so you can see what area will be cropped.
- When marking, you can change the pen color. I think we can live with this one, because you can mark many spots with different colors, and reference them in comments.
- I was tempted to add a pen width selector, but I gave up because I think it does not add much value to deserve UI space. The default 4 pixels width looks good for marking purposes.
- Regarding undo support, there is a "Reset" button, which will reset the image to initial state, clearing all markings, but preserving the crop selection. I think this is enough because usually you don't do many drawings.
- Also, fixed some resources leaks. I ran sleak this time to ensure.
- Fixed a minor bug, where you were able to click finish right on screenshot page, but it gives an bugzilla error because you are supposed to fill the attachment description on next page. This is not permitted anymore.

The mark tool actually uses the "hand" system cursor. I think it would be better to have a "pen" custom cursor. Unfortunatelly my drawing skills are bad, so anyone who wants to contribute a "pen" cursor is welcome. Also, we need icons for the new buttons.

There is a TODO: regarding the "pen" cursor. I can add the code to load the cursor if you want, as soon as we have one.
Comment 6 Willian Mitsuda CLA 2007-11-17 15:55:56 EST
Created attachment 83156 [details]
mylyn/context/zip
Comment 7 Eugene Kuleshov CLA 2007-11-17 16:44:24 EST
Created attachment 83160 [details]
example

Nice stuff Willian. Few coments after playing with this for a bit (not only related to markup):

- because crop is ungrayed when resizing/moving it is hard to see the selected area. perhaps it should not be ungrayed
- same when Mark is active, crop is ungrayed and hardly noticeable on busy screenshots
- would be handy to be able to use Ctrl-Drag to scroll image (without going to the scroll bars)
- both, reset image and capture screenshot actions should ask confirmation
- Esc closes the whole wizard and you lose the all results, so it should ask for the confirmation too
- description text in the wizard title need some work. current text is confusing: "After capturing, drag the mouse to crop. This window will not be captured. Note that you can continue to interact with the workbench in order to set up the screenshot."
- there is something wrong with capture itself, though I run it butstrapped and that could be the reason, but please keep an eye on this (see attachment, the screwed window on the right is the butstrapped Eclipse workbench parent for attachment wizard). This is also why I can't wait when images from clipboard will be supported
Comment 8 Willian Mitsuda CLA 2007-11-18 00:02:50 EST
(In reply to comment #7)
> - because crop is ungrayed when resizing/moving it is hard to see the selected
> area. perhaps it should not be ungrayed
> - same when Mark is active, crop is ungrayed and hardly noticeable on busy
> screenshots

The actual was intentional, because I was worried that if it remained grayed, you could not see clearly.

But it is not written in stone, since this is a new feature and we are still experimenting.

I'll post a second patch, with the ungraying stuff turned off, so you can experiment it.

> - would be handy to be able to use Ctrl-Drag to scroll image (without going to
> the scroll bars)

+1. I've been thinking about it too. I opened bug#210189 for that. I think it is worthy to implement, and not so complex, but it is better to track separately.

> - both, reset image and capture screenshot actions should ask confirmation
> - Esc closes the whole wizard and you lose the all results, so it should ask
> for the confirmation too

Not sure if it is really necessary.

I don't remember any other wizard asking confirmation to cancel. I would go for UI consistency in this case.

Also, they are not actions you would hit accidentally, and even if so, I guess you won't lose much work.

> - description text in the wizard title need some work. current text is
> confusing: "After capturing, drag the mouse to crop. This window will not be
> captured. Note that you can continue to interact with the workbench in order to
> set up the screenshot."

That was Mik's text. I forgot to update it.

> - there is something wrong with capture itself, though I run it butstrapped and

Wow, this is weird. Was it happening before? I didn't change much of the capture code. I just removed a unnecessary asyncExec and introduced a second image buffer to host the user paintings.
Comment 9 Willian Mitsuda CLA 2007-11-18 00:42:27 EST
Created attachment 83175 [details]
Alternative patch

This is an alternative patch.

- It does not "ungray" while resizing/moving crop selections and marking.
- I opted to experiment with "graying on demand" as user selects the crop area. This produces an interesting visual effect.
- The disadvantage is that it obscures user's vision when selecting/resizing/moving; the workaround is to click out of the selection to "uncrop".
- I didn't test yet with dark screenshots.
- It has the advantage of not using the XOR-style selection.
Comment 10 Willian Mitsuda CLA 2007-11-18 00:42:30 EST
Created attachment 83176 [details]
mylyn/context/zip
Comment 11 Eugene Kuleshov CLA 2007-11-18 01:07:21 EST
(In reply to comment #8)
> > - both, reset image and capture screenshot actions should ask confirmation
> > - Esc closes the whole wizard and you lose the all results, so it should ask
> > for the confirmation too
> 
> I don't remember any other wizard asking confirmation to cancel. I would go for
> UI consistency in this case.

as I've been pointing out this thing is like a little editor

> Also, they are not actions you would hit accidentally, and even if so, I guess
> you won't lose much work.

I've actually added it to the list because I did unconsciously hit Esc because crop UI reminded me some other image editors. Also, I disagree about loosing work. Much or not much, work will be lost and it should not be happening because of the accidents.

BTW, Willian, why don't you allow to resize cropped area not only on those special points, but on any point at the edges?
Comment 12 Willian Mitsuda CLA 2007-11-18 23:45:46 EST
(In reply to comment #11)
> as I've been pointing out this thing is like a little editor
> 
> > Also, they are not actions you would hit accidentally, and even if so, I guess
> > you won't lose much work.
> 
> I've actually added it to the list because I did unconsciously hit Esc because
> crop UI reminded me some other image editors. Also, I disagree about loosing
> work. Much or not much, work will be lost and it should not be happening
> because of the accidents.

I think this is somewhat subjective. If you consider the amount of work you may take while going through the wizard, then you would say that most wizards should be asking confirmation.

For example, take the New Java Project wizard, the step when you can configure the project classpath may require a lot of work if you are configuring a project with a lot of dependencies.

> BTW, Willian, why don't you allow to resize cropped area not only on those
> special points, but on any point at the edges?

I suppose this is a common idiom in graphical tools. Also those grab points indicate they are there to be, er..., grabbed! Wouldn't be strange to grab the line itself?

Also, we can consider increasing the size of those points a little (actually they are 5 pixels sided).
Comment 13 Eugene Kuleshov CLA 2007-11-19 01:17:53 EST
(In reply to comment #12)
> I think this is somewhat subjective.

Sure it is, but you just said yourself that you had to take multiple screenshots before you get it right. After losing it you'll have to start over.

> For example, take the New Java Project wizard, the step when you can configure
> the project classpath may require a lot of work if you are configuring a project
> with a lot of dependencies.

Well, it is a good candidate for the enhancement request. :-)

> I suppose this is a common idiom in graphical tools. Also those grab points
> indicate they are there to be, er..., grabbed! Wouldn't be strange to grab the
> line itself?

I checked Picasa and Photoshop. The former don't show drag points and simply allow to resize by dragging edges, the latter does show dragging points, but it still allow to resize by dragging edges.
Comment 14 Willian Mitsuda CLA 2007-11-24 15:47:18 EST
 (In reply to comment #13)
> > I suppose this is a common idiom in graphical tools. Also those grab points
> > indicate they are there to be, er..., grabbed! Wouldn't be strange to grab the
> > line itself?
> 
> I checked Picasa and Photoshop. The former don't show drag points and simply
> allow to resize by dragging edges, the latter does show dragging points, but it
> still allow to resize by dragging edges.

I created bug#210842 to track this separately.
Comment 15 Mik Kersten CLA 2007-11-27 00:56:19 EST
Willian: good stuff, I think that you've got the core support in there, but we need to iterate on the UI in order to make it simple enough.  For future enhancements of this sort I would like to request that you propose the UI before the patch in order to avoid needing to go back on implementation (see http://www.eclipse.org/mylyn/contributors/ ).

This patch does a good job in keeping the functionality needed to markup the screenshot minimal and is promising in terms of avoiding the full-blown image editor space that we want to avoid.  I experimented with various markup and the pen width looks good to.  However, the current experience is a bit overwhelming, and makes it hard to navigate the modality of the actions.  Here's how I propose simplifying, let me know what you think.

* Split this into two pages, one for Capture & Crop, one for Annotate.
* On the Capture page, we just need a "Capture Desktop" and "Actual Size" button (or "Fit Image").  Once the user clicks Next on this page the cropped selection becomes the canvas on the Annotate page.  
* The Annotate page has a single pen icon, with a drop down menu of 8 colors.  I'll dig up a suitable pallete of markup colors.  The other button it has is an Erase All button (currently Rese Image).  Neither of these buttons have text (you can just use arbitrary icons from TasksUiImages for now, I can provide them later.

That should do it.  The modality gets captured by the wizard page, and the user gets to preview exactly what they're submitting.  Clicking "Back" will discard the annotations (and perhaps warn the user).  The Annotate page will probably still need an Actual Size button.  Thoughts?
Comment 16 Eugene Kuleshov CLA 2007-11-27 01:28:34 EST
Mik, why does it have to be split between two pages? What happens if you need to step back and resize selection?
Comment 17 Mik Kersten CLA 2007-11-27 10:30:53 EST
I tried to explain the purpose of the split in comment#15 and answered your second question there.  

In case the reason for the suggested split wasn't clear, it is that the actions of cropping and the actions of markup are different, and require two different modal UI (cropping actions vs. drawing actions).  If it's one wizard page, the UI has to be extremely obvious, will take considerably more work.  If it is two pages, the modality is captured by the pages.  In addition, I expect the cropping use case to be considerably more common than crop+annotate, and splitting into two pages makes the annotate functionality feel separate and optional.  Additional reasons are listed in comment#15.
Comment 18 Mik Kersten CLA 2007-11-27 10:43:12 EST
Willian: Note that the proposal in comment#15 overlaps with bug 209897.
Comment 19 Willian Mitsuda CLA 2007-11-27 11:39:44 EST
(In reply to comment #15)
> Willian: good stuff, I think that you've got the core support in there, but we
> need to iterate on the UI in order to make it simple enough.  For future
> enhancements of this sort I would like to request that you propose the UI
> before the patch in order to avoid needing to go back on implementation (see
> http://www.eclipse.org/mylyn/contributors/ ).

OK. From my side, I don't mind changing or dropping patches, because I find it easier to iterate using code and experimenting proposals, mainly when the enhancement involves UI usability.

> * The Annotate page has a single pen icon, with a drop down menu of 8 colors. 

Agree. A pre-selected set of colors looks simpler than the color selection dialog.

But what control are you planning to use? I thought about a combo with images representing colors, but this is not supported by SWT: bug#93809 and bug#4510.

(In reply to comment #17)
> If it's one wizard page, the
> UI has to be extremely obvious, will take considerably more work.  If it is two
> pages, the modality is captured by the pages.  In addition, I expect the
> cropping use case to be considerably more common than crop+annotate, and
> splitting into two pages makes the annotate functionality feel separate and
> optional.  Additional reasons are listed in comment#15.

I think the opposite: adding more pages make the process look more complex. In which aspects the current, 1 page UI, is not obvious?

Also, note that the in the current patch, the crop function comes preselected, and if you don't want to annotate, just ignore it and move to next page.

(In reply to comment #18)
> Willian: Note that the proposal in comment#15 overlaps with bug 209897.

OK, perhaps we can think of how "preview" will be implemented as a side effect of what we decide here.


BTW, have you experimented booth the patches? What do you think about the "ungraying" problem Eugene mentioned in comment#7, and what do you think looks better?
Comment 20 Mik Kersten CLA 2007-11-27 11:58:36 EST
 (In reply to comment #19)
> OK. From my side, I don't mind changing or dropping patches, because I find it
> easier to iterate using code and experimenting proposals, mainly when the
> enhancement involves UI usability.

As long as you don't mind going back on some implementation I don't mind either.  This is the way I work too :)

> Agree. A pre-selected set of colors looks simpler than the color selection
> dialog.
> 
> But what control are you planning to use? I thought about a combo with images
> representing colors, but this is not supported by SWT: bug#93809 and bug#4510.

I was just thinking of a toolbar item with a pull-down menu (like the Task List view's Presentations) and then hard-coding the actions' images to be the colors.  We could consider a dynamic menu (e.g. like Context Highlighters in http://www.eclipse.org/mylyn/new/new-0.3.html ) but I don't think it's worth it.  

However, with the separate Annotate page we can get rid of the drop-down alltogether, and just have an action group with 6 or 8 pen colors (only one of which can be selected) and then a separator, then an Erase All button.
 
> I think the opposite: adding more pages make the process look more complex. In
> which aspects the current, 1 page UI, is not obvious?

Yes, more pages is definitely more complex if one page can suffice, and we should always keep in mind that adding a page is a major addition in complexity.  However, I'm pretty much convinced that we need a preview page anyway, because the user needs to see the final image that they submit.  Also, the fewer buttons the better.

What makes the single toolbar more complex is that it mixes tools for modality with tools for operations.  Good image editors have to be very careful about making that separation (I can point you to Allan Cooper's literature about how Photoshop achieves this with having separation between a tool palette, tool options, and other operational controls).  I think that implementing such a separation on this wizard page is going to result in more complexity than having a separate Annotate page.  The Annotate page removes the need for the modality controls/buttons entirely, because the page you're on determines whether the cursor is being used to crop or to paint.  In other words, we effectively end up with 2 pages and 4 buttons for this entire UI, which is great:
* Capture+Crop page: Capture Desktop, Actual Size
* Annotate page: pen selector button group, Erase All
Comment 21 Willian Mitsuda CLA 2007-11-30 13:49:31 EST
 (In reply to comment #20)
> I was just thinking of a toolbar item with a pull-down menu (like the Task List
> view's Presentations) and then hard-coding the actions' images to be the colors.

That's the problem. I'm wondering if it would look good to incorporate a toolbar inside a wizard page.

> The Annotate page removes the need for the modality
> controls/buttons entirely, because the page you're on determines whether the
> cursor is being used to crop or to paint.  In other words, we effectively end up
> with 2 pages and 4 buttons for this entire UI, which is great:
> * Capture+Crop page: Capture Desktop, Actual Size
> * Annotate page: pen selector button group, Erase All

What if the user makes some annotation and wants to adjust the crop area again? How will we handle it? Erase the annotation every time the user goes back and change the crop area?

I think that optmizing the "toolbar" buttons are a good argument to split the page, but I'm no so convinced if it is worth sufficiently to simplify user experience. At first look, it sounds we are making the process more bureaucratic.
Comment 22 Eugene Kuleshov CLA 2007-11-30 15:21:01 EST
Willian, welcome to the club. :-)
Comment 23 Mik Kersten CLA 2007-12-03 20:25:06 EST
Willian: the way I see it is that we either do the split we end up with a simple and toolbar-free UI.  If we don't we end up with a complex UI, perhpas to the point where we have to consider making an editor and support operations like undo.  So the question is, what value does the more complex UI provide?  The main thing I see it providing is the following: the user can re-crop without losing annotations.  My judgment was that this would be a very rare use case, because the workflow of a screenshot tends to first capture and crop the entire area of interest, then to annotate that area.  Are you seeing this as a more common use case?  Are there other use cases suitable for a wizard that we should consider?  
Comment 24 Mik Kersten CLA 2007-12-05 18:50:17 EST
Tentatively scheduling for 2.2, since most of the core work has been done and we just have this one UI thing to figure out. 

Willian: shall I assign to you?  Also, if you think it would be best to have some design discussion about how best to approach this, you could consider joining us for the next conference call where we can bring this up with other committers, since it's a tricky choice.
Comment 25 Mik Kersten CLA 2008-01-03 12:23:41 EST
Willian: any thoughts on comment#23?  This is a valuable contribution, and I would like to proceed on integrating it.  If you feel strongly against the toolbar approach we can reconsider, but also let me know if your thinking then is that we move to more of an editor style UI (even if the editor is embedded in a Wizard page) and what your thinking on the preview page is.
Comment 26 Willian Mitsuda CLA 2008-01-22 01:34:32 EST
Hi Mik,

Sorry for my slowness on this, I've been quite busy these days, but, yes, you can assign to me. I might have time to work on this from now on.

After attaching some screenshots, I'm convinced for the need of the preview page. I think it is like when you are shopping online, and the last step presents you with a report of all items you are buying, you credit card number, shipping address, etc., and the finish button.

Submitting a screenshot is quite the same, as you are submitting a sensitive information which may contain some information you don't want to expose, so it needs to be previewed in the last step, even if it sounds redundant.

Regarding the split question, I still think the better is to maintain just one page. With crop being the initially selected "tool", the "mark" end up being entirely optional and the user won't even notice it if he needn't.

So, the workflow would be: capture [crop] [mark] -> fill attachment description -> preview -> finish.

Regarding the toolbar question, I'll make some tests with toolbar control and post screenshots here. But anyway, as a last try, I think we can stay with a row of buttons, as is in the current patch, as long as we don't add more.

But before, I'm worried about 2 related requests:

1 - Generalizing screenshot facility for ECF
2 - bug#155323 - Image editor (Eugene)

I read the thread in mylyn-dev, and I'd like to know if you plan to make some change to the workflow that may affect the ongoing work. I guess (by the discussion) the screenshot attachment wizard will have to be split into (a) a screenshot capture facility and (b) a set of actions to consume the captured screenshot (attach to task, save as image in workspace, send as IM).

Perhaps isn't it time to rework the entire screenshot workflow? Actually it is:

1 - Start screenshot attachment
2 - Capture
3 - Edit
4 - Attach

The workflow is very tied to attachment itself. I could say the same for the "Send screenshot via IM" prototype from ECF:

1 - Start sending screenshot
2 - Capture
3 - Edit
4 - Send to another user

A more natural and flexible workflow would be (IMHO):

1 - Capture
2 - Edit
3 - Action

Step (1) would be accomplished by a small dialog with capture options: desktop, window, timed capture. It is like the Jazz screenshot Eugene posted in his blog and it is how it is done in GIMP, for example.

Step (2) would be made inside an basic image editor, where the user can crop/mark/etc. It could be a very basic image editor provided by Mylyn, or a more advanced provided by a third party.

Step (3) would be contributed by extensions, one of them being attach to task by Mylyn, send via IM by ECF, etc.

What do you think?
Comment 27 Mik Kersten CLA 2008-01-31 20:19:35 EST
Great summary Willian.  Regarding generalizing this, I think that you outline some important aspects of the design space.  However, for the time being I suggest that you ignore those and just get the markup into what we currently consider to be the best workflow for the Task Editor.  Then during a second pass we can generalize when we get more input from the other clients interested in this functionality.  Practically speaking this means that our goal should be to apply your patch for markup, then once that's finished and we have learned more, as we always do, we can with generalization via bug 214379.  

I also like your summary of why the preview step is important.  I'm still a bit concerned with having crop and mark on the same page, but if you think it's best that's fine.  So have we converged on the following as the next pass at the workflow?
1) Page 1: capture -> crop -> mark
2) Page 2: fill details (standard page for all attachments)
3) Page 3: preview -> submit (standard page for all attachments, extended to support image preview)
Comment 28 Willian Mitsuda CLA 2008-02-01 14:18:33 EST
Sounds fine for me.
Comment 29 Willian Mitsuda CLA 2008-02-04 22:05:27 EST
Created attachment 88850 [details]
Screenshot

Prototype UI using a toolbar
Comment 30 Willian Mitsuda CLA 2008-02-04 22:32:42 EST
Created attachment 88853 [details]
Another UI prototype screenshot

Another toolbar alternative: icons with text on the right.
Comment 31 Willian Mitsuda CLA 2008-02-04 22:36:07 EST
Created attachment 88854 [details]
Another toolbar prototype: text under icons
Comment 32 Willian Mitsuda CLA 2008-02-04 22:42:56 EST
I just posted some toolbar experiments I made. It uses a ViewForm to position the whole set. It is the same control used to layout the view contents, and the mini toolbar inside the new java project wizard.

I particularly liked the 2nd. It doesn't waste too much space and doesn't appear to be so hidden like the 1st.

Please, ignore the horrible dummy icons I made for this experiment :) This prototype also lacks disabled version of icons.

Let me know what you think.
Comment 33 Eugene Kuleshov CLA 2008-02-04 23:44:53 EST
(In reply to comment #32)
> I particularly liked the 2nd. It doesn't waste too much space and doesn't appear
> to be so hidden like the 1st.

I also like the 2nd too (maybe "Select Color" would be better then "Change Color"), though that extra border around ViewForm seem redundant, since page don't have any other content unlike the new java project wizard. So here border only adds visual noise.
Comment 34 Mik Kersten CLA 2008-02-07 22:36:59 EST
Willian: Great stuff!  The 2nd is my favorite too.  We're all set to proceed with the patch because I figure that finessing it more will require playing with the UI some.  Please don't worry about icons or labels right now, I can iterate on those.  The only question I have is how will the user trigger the "Crop" action after selecting that tool?  In Photoshop this is not quite discoverable beause you have to hit Enter.  I think that we can go with that but we could help the user by putting an Info message on the wizard dialog saying something like "Press Enter to crop" after the user has made the crop selection.
Comment 35 Willian Mitsuda CLA 2008-02-08 12:04:37 EST
OK, I'm just finishing the color selection drop-down UI and I'll post an updated patch soon.

Regarding the crop, it is transparent to the user. There is no need to "confirm" the crop. When you hit next, if there is an region selected, that will be the cropped region.
Comment 36 Willian Mitsuda CLA 2008-02-18 20:56:00 EST
FYI, I'm almost done. I'm doing some refactoring due to necessary changes for the preview stuff.

Mik, let me know if there is some deadline for 2.3.
Comment 37 Mik Kersten CLA 2008-02-19 02:53:41 EST
Great.  Wednesday is the deadline for any new functionality.
Comment 38 Willian Mitsuda CLA 2008-02-20 13:42:46 EST
Created attachment 90227 [details]
Patch
Comment 39 Willian Mitsuda CLA 2008-02-20 13:43:19 EST
Created attachment 90228 [details]
mylyn/context/zip
Comment 40 Willian Mitsuda CLA 2008-02-20 14:03:54 EST
The attached patch is a little big, so feel free to ask any questions.

There are some TODOs in ScreenshotAttachmentPage regarding icons and cursors.

For the color palette, I used the classic 16 color EGA palette (RGB color codes obtained from wikipedia).

The preview was a little tough because I wanted it to be exactly the same image that would be attached, so you can see the loss of resolution in JPEG, etc. But this introduced a new category of problems the current LocalAttachment model was not prepared:

- To obtain maximum fidelity, the screenshot image has to be converted into the proper file format using ImageLoader before previewing or attaching.
- ImageLoader.save is a expensive operation, so it has to be done lazily, and only if needed, i.e. if you go into preview, go a page back and returns to preview without modifying anything, it does not have to convert the image again. The same is valid if you go to preview and them attach; there is no need to convert again.
- If you go to preview, go back to screenshot editor, modify it, the next time you go to preview or attach, it has to convert it again.

Initially I was planning to refactor LocalAttachment and use the opportunity to cleanup some code smells in it (like the setFile, setFilename, setFilePath; split clipboard and file handling), but it turned out to be very risk since I was touching a lot of code of generic attachment wizard. So I ended up making an ugly ImageAttachment subclass of LocalAttachment, overriding the behavior I need.

It works well, and doesn't hurt the existing wizard. But keep in mind it will be good to do some refactoring in the LocalAttachment in the future.

Another little issue is with the existing preview page, that expands itself to the size of image. This can be very annoying when attaching the entire desktop or big parts of it. I logged bug#219637 for handling this later.
Comment 41 Eugene Kuleshov CLA 2008-02-20 14:50:34 EST
Willian, does attaching image from the clipboard fits into your UI?
Comment 42 Willian Mitsuda CLA 2008-02-20 15:24:54 EST
 (In reply to comment #41)
> Willian, does attaching image from the clipboard fits into your UI?

For the attaching from clipboard story we can modify the traditional attach wizard, with the NewAttachmentPage sniffing an Image from clipboard, and then presenting the ScreenshotAttachmentPage as the next step (without the "capture desktop" button).

But one problem here would be discoverability (attach X attach screenshot).
Comment 43 Eugene Kuleshov CLA 2008-02-20 17:35:57 EST
that would be really neat. For discoverability we can expand "Clipboard" label and list supported content types like "Clipboard (text or images)"
Comment 44 Willian Mitsuda CLA 2008-02-21 00:32:18 EST
Created attachment 90286 [details]
Patch for color selection UI
Comment 45 Willian Mitsuda CLA 2008-02-21 00:35:45 EST
Created attachment 90287 [details]
Patch for mark up functionality
Comment 46 Mik Kersten CLA 2008-02-21 01:07:40 EST
Beautiful!

Patch applied.  The icons don't come across in patches, so unless I hear otherwise I'll create those.  I'll iterate on this tomorrow and report back.
Comment 47 Willian Mitsuda CLA 2008-02-21 01:17:06 EST
My icons were just for tests and are not production quality :)

I'm sure yours will be better.
Comment 48 Mik Kersten CLA 2008-02-21 01:31:25 EST
Production icons added :)  Let me know if any of them are unclear.
Comment 49 Eugene Kuleshov CLA 2008-02-21 01:59:32 EST
 (In reply to comment #48)
> Production icons added :)  Let me know if any of them are unclear.

I'd say that "cut" is rather confusing as cut usually deletes selected artifact after sending it into the clipboard, but I thought in this case we have "crop" operation.
Comment 50 Steffen Pingel CLA 2008-02-21 02:30:47 EST
The screenshot editing and preview is awesome! The only minor nit I have is that there is no way to actually crop the image. Did you consider adding an "Apply Crop" action that would limit the visible portion of the image to selection? I kept hitting Enter which took a new screenshot.

Some ideas for refactoring the data model of the attachment wizard have been discussed on bug 193156 comment 15 and following. I would like to do that for 3.0 and also consider offline support / batching attachment submission. Please file a new bug with the requirements you have learned from resolving this bug and we can iterate further.
Comment 51 Mik Kersten CLA 2008-02-21 17:50:52 EST
The Apply Crop thing is tricky because it brings in the question of Undo.  I think we should put it out in the current form for 2.3 and consider changes for 3.0.

This came in a couple of hours after our desired feature freeze, so we also need to give it some additional testing before it goes out.  But the core work is done.  I'll consider whether I can find or generate a better icon than the scissors for cropping when doing final UI review.

Willian: I considered whether we should get rid of one of the rows of the color picker to keep things simple.  But I like how you chose the light and dark colors and for dark screenshots the light ones could be worth keeping.  But should we get rid of the Close button?  It seems obvious that the clicker disappear after color selection or other click.
Comment 52 Willian Mitsuda CLA 2008-02-22 01:27:03 EST
 (In reply to comment #49)
> I'd say that "cut" is rather confusing as cut usually deletes selected artifact
> after sending it into the clipboard, but I thought in this case we have "crop"
> operation.

+1 for finding a better alternative to "cut".

 (In reply to comment #50)
> The screenshot editing and preview is awesome! The only minor nit I have is that
> there is no way to actually crop the image. Did you consider adding an "Apply
> Crop" action that would limit the visible portion of the image to selection? I
> kept hitting Enter which took a new screenshot.

Hum... I think I'm finally understanding what Mik referred in comment#34. So, you two are thinking in terms of a "activate crop - select - apply" cycle, with "apply" effectively replacing the whole image with just the cropped part, right?

I was just following the concept which came since the 1st version of the screenshot wizard which is "if something is selected, submit just that; otherwise, submit entire image". I thought it was so intuitive that I wasn't understanding exactly what Mik mean.

> Some ideas for refactoring the data model of the attachment wizard have been
> discussed on bug 193156 comment 15 and following. I would like to do that for
> 3.0 and also consider offline support / batching attachment submission. Please
> file a new bug with the requirements you have learned from resolving this bug
> and we can iterate further.

OK, I'll read that later and post another bug for LocalAttachment refactoring.

 (In reply to comment #51)
> The Apply Crop thing is tricky because it brings in the question of Undo.  I
> think we should put it out in the current form for 2.3 and consider changes for
> 3.0.

OK, so we'll have time to get more feedback from users, and change if need.

> Willian: I considered whether we should get rid of one of the rows of the color
> picker to keep things simple.  But I like how you chose the light and dark
> colors and for dark screenshots the light ones could be worth keeping. 

No need to thank me. Thanks whoever created the EGA palette :)

http://en.wikipedia.org/wiki/Enhanced_Graphics_Adapter

> But should we get rid of the Close button?  It seems obvious that the clicker
> disappear after color selection or other click.

I was in doubt, but if you think it is so obvious, go ahead.
Comment 53 Eugene Kuleshov CLA 2008-02-22 05:21:25 EST
Created attachment 90454 [details]
ms word color picker and crop icon

It seem I am the only one who wind those colors odd. Here is how MS Word color picker look like. There is also a crop icon on the picture toolbar, just to give an idea.
Comment 54 Eugene Kuleshov CLA 2008-02-22 05:31:10 EST
Couple more things: 

- toolbar is visually too close to the picture area and it don't stand apart from the picture, especially when working with large images
- annotate button seem redundant and maybe it is better to combine it with the color chooser dropdown
Comment 55 Willian Mitsuda CLA 2008-02-22 13:25:31 EST
Eugene, feel free to suggest a better palette.  I can also modify the color picker if you all agree on a common layout.
Comment 56 Mik Kersten CLA 2008-02-28 17:00:15 EST
Eugene: the purpose of the XP color pallette is considerably different than screenshot markup.  I find the pallette Willian picked a great choice for reasons mentioned (thanks for the EGA pointer Willian :)

Willian: could you also make a new bug for the cropping portion of your comment#52?  Yup, Steffen's comment pushes on the concern raised in comment#34.  Also include the cut stuff in that bug, since it's related to the same workflow and has room for improvement.  Although again I want to reiterate that I'm very impressed with how usable the current result is.
Comment 57 Eugene Kuleshov CLA 2008-02-28 18:43:45 EST
(In reply to comment #56)
> Eugene: the purpose of the XP color pallette is considerably different than
> screenshot markup.  

Would you mind to enlighten me?

> I find the pallette Willian picked a great choice for
> reasons mentioned (thanks for the EGA pointer Willian :)

I didn't noticed any specific reasons, last you said that you liked them. Personally I miss plain red and plain blue colors from the currently chosen palette.
Comment 58 Mik Kersten CLA 2008-02-28 19:57:32 EST
Given my experimentation marking up a series of screenshots, the chosen light colors work well on dark regions and chosen dark colors work well on light regions.
Comment 59 Eugene Kuleshov CLA 2008-02-28 20:19:58 EST
(In reply to comment #58)
> Given my experimentation marking up a series of screenshots, the chosen light
> colors work well on dark regions and chosen dark colors work well on light
> regions.

Mik, you haven't explained what was the purpose of XP palette.

Also, I don't quite get how your experimentation helps with the fact that those are not the colors I as user would like to use for marking up my own screenshots? In my eyes, pastel colors aren't good for highlighting.
Comment 60 Willian Mitsuda CLA 2008-03-08 01:54:45 EST
 (In reply to comment #56)
> Willian: could you also make a new bug for the cropping portion of your
> comment#52?  Yup, Steffen's comment pushes on the concern raised in comment#34.
> Also include the cut stuff in that bug, since it's related to the same workflow
> and has room for improvement.  Although again I want to reiterate that I'm very
> impressed with how usable the current result is.

Opened bug#221952 to track this.
Comment 61 Willian Mitsuda CLA 2008-03-08 02:07:12 EST
 (In reply to comment #59)
> Also, I don't quite get how your experimentation helps with the fact that those
> are not the colors I as user would like to use for marking up my own
> screenshots? In my eyes, pastel colors aren't good for highlighting.

Eugene, I like some of your suggestions. I opened bug#221954 for this. Let's continue the discussion in there.
Comment 62 Willian Mitsuda CLA 2008-03-14 22:10:07 EDT
 (In reply to comment #50)
> Some ideas for refactoring the data model of the attachment wizard have been
> discussed on bug 193156 comment 15 and following. I would like to do that for
> 3.0 and also consider offline support / batching attachment submission. Please
> file a new bug with the requirements you have learned from resolving this bug
> and we can iterate further.

Done: bug#222833.