View and add comments on previous commits in image file view

This is super nice and pretty much the reason to use invision. Perfect for design work.

I don't know if there is an easy way to do this, though.

Added ~12742 ~19492 labels

Added ~122770 label

Added ~123454 label

I like it, it's possible to do with javascript on client side, but I'm not sure about creating preview from source files.

We can parse psd with https://github.com/layervault/psd.rb but we need also: ai, sketch and atype. If we can parse these following formats we can also track visual changes and keep iteration design process with direct feedback on a screen.

Reassigned to @skyruler

Please check attached file

cc @dzaporozhets

@skyruler this issue is about comments on pictures in diff. Not about pictures in issue. Please make a proper mockup

@skyruler example of commit with image is https://gitlab.com/gitlab-org/gitlab-test/commit/2f63565e7aac07bcdadb654e253078b727143ec4

Title changed from Comment on pictures (in-picture-comments) to Comment on pictures (in-picture-comments) in diff

@skyruler consider exploring the examples of Zeplin.io, Phabricator and invisionapp.com first.

@sytses yeah I used Invision and Zeplin a lot, they have a bit different workflow but both are really good example of design driven workflow.

Onion skin

@ModestGoblin hey it's awesome! I have only 1 concern regarding more than 1 comment on the pic, in design that I uploaded above you can see a counter that solves that issue.

Love this.

Assignee removed

Removed ~19492 label

Added frontend label

Milestone changed to 8.7

@jschatz1 is this something to use Vue js for?

This depends on https://gitlab.com/gitlab-org/gitlab-ce/issues/10325, which is due for 8.7. Moving this to 8.8 therefore.

Milestone changed to 8.8

@JobV Sure. Anything can be for vue.

Milestone changed to 8.10

Milestone changed to %Backlog

Another moonshot idea. Maybe we can be more ambitious here and have intricate image diffing.

cc @sytses

@JobV Awesome idea!

Added moonshots label

mentioned in issue #20721 (closed)

This!

related issue somewhat, as it creates automatic previews out of sketch files: https://gitlab.com/gitlab-org/gitlab-design/issues/15

We should definitely support more image/design formats: .psd .sketch etc

I think we should treat this the same as discussions on diffs in merge requests, make them resolvable and show them inside the issue it self as well

note: attention/note dots/points/markers should not obscure the detail they are all about

@JobV maybe the time for this has come? /cc @stanhu

BTW @fatihacet you might like to make this

@ModestGoblin thanks for the blog post!

@sytses I think we can do this 8.14 / 8.15 or so :)

Mentioned in issue #22113 (closed)

@sytses This is an AMAZING feature and I really want to work on this when the time comes. /cc @JobV @jschatz1

@fatihacet noted :)

Mentioned in issue #22518 (moved)

We had a discussion on this today at the UX/Front-end call. Basically it was to enact more discussion on this issue and to already start any possible UX on this.

I will paste my talk points (which I made for myself) at the bottom, just for information. To conclude the information that was discussed:

This issue currently talks about discussion in pictures (only in commits/diffs). This would make no sense to our current conversational design workflow. It should still be added though, however the more pressing matter that should be attended to is the ability to have these kinds of discussions inside in issue discussion.

Meaning we enhance the current images with the functionality to comment inline with them.. You would then be able to see the issues both in the image, as well as chronologically in the issue discussion. Sidenote on this, is that I really like the MR discussion resolve functionality.. which we could use for this as well...

I will come back to this with some mockups in the near future.

there was also talk about having some real functionality for storing images.. rather than just the url to the actual image... etc This still needs more discussion but I am happy it started..

cc: @dzaporozhets @awhildy @jschatz1 @mikegreiling

Comment on pictures (in-picture-comments) in diff

This issue has been on my mind since I started at GitLab.

It is maybe on the roadmap for 8.14 8.15

Basically the original issue says:

"When the diff is a picture I should be able to comment on a specific place in the picture.""

There are a few things to consider though:

In what place is this most useful? We discuss designs in an issue discussion and less probably in a MR discussion.

MR discussions have the possibility to include "diffs". Current issues do not. We "can" have a discussion in a commit.. rarely used, but possible.

Currently our design process for updating/committing a design involves a screencapture or image output from our design files or browser. We then paste this "information" or image inside a discussion comment and voila.. we updated the design, perhaps replacing the "Single Source of Truth" in the original Issue body text.

Although I like the idea of being able discuss an image inside a commit, with the added functionality of pointing inside the image where your comment is based on... I do not think it takes this idea far enough. It wouldn't add to much to our design discussion. We want to have design driven discussion.. And I think not detach that discussion from the issue discussion:

For every design file we would have to commit the image to a repository, then reference it in order for it to become discussable. Another complication is, for community contribution this would make no sense... As they would have to create a repository for design files themselves and repeat the process above.. in order for their images to become discussable as well etc...

Also: A diff inside a MR, is something that is automatic. The referencing of a commit with an image though is then manual.

I think the better way is to make design files (either images.. or even actual design files like .sketch .psd etc) part of the issue discussion... Or rather.. upgrade existing images inside discussions.

painpoints:

what if an image is being discussed on.. but then that image inside a comment is deleted?
how would we invoke 2-up, swipe, onion (revision visualisations)?

Basically the original issue body is not really a moonshot issue...

"When the diff is a picture I should be able to comment on a specific place in the picture.""

This would basically only apply to discussion within a commit.

We should change it to:

"When "

Would this mean we either can mention

Do we want to detach design discussion from general issue discussion? Maybe thats a good thing... however.. everyone should be able to commit to.. present a "next iteration"

@dimitrieh Just something I wanted to saying during the UX call but we ran out of time.

Re: the idea of including this within Issues

I think this feature would clearly be more useful within Issues, as that's where all the design discussion and iteration happens.

I just wanted to point out that it seems like properly tracking & managing the display of versions of images/sketches would need to be an early priority for this feature. I say that because although easier diff discussions will be awesome because we'll be able to have way more discussion and iterate faster, it will also mean we'll more quickly have way too many versions of images (and discussions around them) than a person could reasonably keep track of (or should fit on a page). I already often feel very disoriented coming to an issue where many designs have been posted, discussed, and changed. It's hard to know which version of the designs I should be looking at.

Another way of saying this: given that the purpose of this feature is very similar to the purpose of the 'Changes' tab of an MR, I think it's worth noting that the only way the 'Changes' view works is that every version of every change is associated with a specific commit, and we make sure it's very easy for the user to only view only a single commit at a time, or how it compares to one other commit. So I guess my comment boils down to the idea that it seems (maybe I'm totally wrong about this) that in order to begin treating images as trackable, diff-able, discuss-able resources & separately from MR's, there is a lot of structure we have to put in place to manage the complexity it could create.

I don't know if that makes sense. And maybe everything I said has already been discussed elsewhere.

In conclusion, I think this feature would be absolutely awesome. I just think separating it from MR's (and git/version control in general) will likely introduce a lot of complexity for developers and users alike.

Looks like while I was writing that, you posted a comment that showed you're thinking about this already:

For every design file we would have to commit the image to a repository, then reference it in order for it to become discussable. Another complication is, for community contribution this would make no sense... As they would have to create a repository for design files themselves and repeat the process above.. in order for their images to become discussable as well etc...

Also: A diff inside a MR, is something that is automatic. The referencing of a commit with an image though is then manual.

@dimitrieh @pedroms: Earlier today, Pedro mentioned how InVision and Jira were partnering, and how it made him think about non-intrusive, rich-embeds. I was thinking about this, and wanted to throw another rough concept out there.

Today, our comments are purely chronological. If you are caught up with the conversation, it is nice to have all the comments in one timeline and one place, understanding what happened when, when system messages occur, etc. However, if you are trying to catch up, the comments are overwhelming, and it is hard to follow what relates to what. What if you could 'filter' down the conversation just to those comments that relate to an image (in this case). Put together a very rough exploration: http://codepen.io/awhildy/full/wzjEor/

The 'filtered' view might even be able to include different versions of the image. A downside is that the long conversation is still hard to parse until you filter it. Also isn't clear if you can reply to a comment on a photo. Still a lot of open questions with this idea, but thought I would just throw it out there Thoughts?

@awhildy Thanks for putting this together, really sparking ideas on my end! I think it's necessary to filter comments related to a specific image, or else it will be difficult to keep up.

Picking up on what @brycepj wrote:

I already often feel very disoriented coming to an issue where many designs have been posted, discussed, and changed. It's hard to know which version of the designs I should be looking at.

Image versions (and discussions around specific versions) are a great challenge to tackle and related to comments on images. I think the concerns you are voicing are mainly (but not only) due to way images are shown in issue comments. Maybe, we can work on theses issues in three steps:

1) Improve image previews/thumbnails on issue comments and image viewing

2) Add comments on images as a regular thread

For these two, take a look at how Basecamp previews files and rich media using a grid, which you can then click on to enlarge and comment on them:

We could have something similar, with a comment count badge near the image thumbnail, so you would know where to look. Notification URLs and mentions inside those image comments would automatically open the image preview and jump to the linked comment.

3) Evolve comments on images to discussions around specific image points

This is what started this discussion, similar to what InVision, Zeplin and other already do (example: https://support.invisionapp.com/hc/en-us/article_attachments/204211666/PostComment2.gif)

I believe it's easier to break this down into smaller chunks as described, but feel free to disagree with me and propose the contrary

Mentioned in issue #23482 (closed)

Changed title: Comment on pictures (in-picture-comments) in diff → Comment on pictures (in-picture-comments)

Mentioned in issue #23547 (moved)

Mentioned in issue gitlab-com/www-gitlab-com#871 (closed)

Added ~19492 label

Finally I have some time to get back to this very nice issue . I really want to get this in as this will be amazing!

The stuff I will be writing down tries to include everything that has been said previously!

We basically have 2 places where we want to fix comments in images (let's treat them separately as they are very different):

Pictures inside a commit
Pictures inside a comment

edit:I split this comment in to separate comments below!

Pictures inside a commit

A picture inside a commit is something on which we need to be able to comment in order for it be discussable either on the commit itself or inside a MR (of which the commit is part of). Currently that is not possible, it is however possible for us to discuss a specific line in detail within a code/text file. The same should be possible for an image, we should be able to mark/point inside the image where our comment is targeted at.

Comment on a code line example:

I setup a repository to give an example of all the different commenting examples that are currently possible: https://gitlab.com/dimitrieh/comment-on-pictures

So currently the following things are possible for commenting on a commit:

Comment on a whole commit - example
Comment on a code line in a certain file in a commit - example

If that commit is part of a Merge Request, these comments will be visible there as well, plus a third option will become available, listing:

Comment on a whole commit, visible within the discussion on a Merge Request - example
Comment on a code line in a certain file in a commit, visible within the discussion on a Merge Request - example
Comment on a code line in a certain file in a diff of the Merge Request - example

As we can see in the examples above, is that if a comment is displayed within the discussion of a Merge Request, it takes the form of a sub discussion. A discussion that does not follow time linearity anymore and thus becomes focused and as a bonus can be resolved if the comment is in a diff!

Imo this UI can also be used to display image discussions inside a Merge Request discussion, similar to how code line comments work!

As we are mostly interested in images, let's list how they are currently displayed if a commit includes an image or multiple image files:

New image(s) in a commit - example
New but changed image(s) within the same Merge Request in a commit - example
Existing changed image(s) in a commit - example (basically the same as the previous point)
Existing changed image(s) in a commit, visible within the diff of a Merge Request - example

We can conclude that if an image already exists, and is basically changed by a commit, we will automatically see a 2-up/swipe/onion skin sub diff displayed!

So what we want to achieve is the following:

Comment on a part of a certain image file in a commit
Comment on a part of a certain image file in a commit, visible within the discussion on a Merge Request
Comment on a part of a certain image within the diff of the Merge Request.
If the image displays a 2-up/swipe/onion skin sub diff , we want our comment target to be correctly reflected in all 3 views.

Implementation

We will have 3 ways of commenting:

Comment on the whole image in general
Comment inline in the image with a target
Reuse an existing target within an image and comment on it inline or by mentioning the target ID

For the target ID we would need another mention or special gitlab reference character like we use for mentioning users/merge requests, issues etc!

I propose the following syntax: ^1 or &1 or (1) (although the last one would work slightly different)

I mocked up an example of how it would look like on the following page: https://gitlab.com/dimitrieh/comment-on-pictures/merge_requests/2/diffs#diff-0

Mockup! (click on it, if isn't diplayed big)

Inline commenting example mockup:

As told previously we would like to reuse the UI of the sub discussions inside a merge request discussion. That will look like this:

sub discussion mockup:

As these diffs can become unreasonable large we would like to show them collapsed by default or a show only tiny part of them. When expanded the 2-up/swipe/onion skin will become available!

Totally collapsed:

Showing tiny portion:

Fully expanded with 2-up/swipe/onion skin available:

Pictures inside a comment

Moved to separate issue https://gitlab.com/gitlab-org/gitlab-ce/issues/24816 !

Let's use this issue for comments on pictures in commits!

moved to #24816 (moved)

Mentioned in issue #24344 (closed)

Mentioned in issue #24695 (closed)

Mentioned in issue #24816 (moved)

Changed title: Comment on pictures (in-picture-comments) → Comment on pictures inside a commit (images as assets)

cc @victorwu

Added code review label

Thanks @dimitrieh for the all the detailed designs and thorough explanations. I really like how you've carefully separated the problems into the two scenarios.

I think the commit / merge request diff scenario is pretty awesome in concept. And it would make a great demo and people would be blown away if they try it out in GitLab. It may be the future of design collaboration. (What's your prediction?) But it's pretty risky because we would spend a lot of resources building it and we may not get a lot of usage.

The second scenario you are working in the other issue is similar in concept to the JIRA-InVision workflows right? Collaboration is mediated within the issue comments with discrete uploaded images. This seems like a pretty standard workflow that designers would like today. I'm sure we would get a lot of usage if we ship this latter scenario.

So let's focus on the latter scenario first. I'm not ready to invent the future just yet for designer-focused features in GitLab. (But it's clearly a wide open opportunity in the marketplace.)

Removed ~123454 label

Milestone removed

@victorwu the images within commit commenting system is actually fully thought out I think.. thus ready to be implemented, making use of existing UI. Making the task of implementing it less daunting. Also I have already come across various times where this functionality would have come in handy. Also this has been the original content of this moonshot issue since a year ago!

This part of the comments on images is not actual design discussion (which happens in issue threads) but rather for asset discussion and implementation purposes. As it discusses images within commits/within final products/repos. It would be cumbersome to first commit an image in order for it to be discussable in a normal design discussion. However for example for the gitlab.com repository https://gitlab.com/gitlab-com/www-gitlab-com this feature would come in very handy, also for https://gitlab.com/gitlab-com/gitlab-docs agreed @axil ?

Therefore I strongly suggest scheduling this first and actually implementing this. GitLab already contains functionality for diffing images, with this feature it actually/finally would become useful!

My second point towards this, is that indeed the actual design discussion implementation of comments on images within issue discussions, which is moved to https://gitlab.com/gitlab-org/gitlab-ce/issues/24816, (we really want this as well!) still needs quite some discussion for it to be doable at all.

My hope is we can schedule this issue https://gitlab.com/gitlab-org/gitlab-ce/issues/2641 for 8.16 and #24816 (moved) for 8.17.

Please consider this to be what it is, important!

I would love to discuss this issue and #24816 (moved) in more detail with a meeting to perhaps better explain the implications this will have from my perspective

Milestone changed to %Backlog

Chatted with @dimitrieh and he gave some great insights. Will summarize our discussions in the description soon, and scope out the first items we need to work on to make this a reality.

Mentioned in issue #24949 (closed)

Added to schedule label

@DouweM : Is it possible to the merge request diff version of this? As I understand it, a merge request diff is a diff between two branches, and we store any comments on the diff itself (and they are not associated with commits, etc.) Is this accurate?

If so, would commenting on the merge request diff of two branches on an image be straightforward? I.e., if we wanted to comment on a particular spot in the image diff, just store the (x,y) coordinates? If this is straightforward, it seems like a good win to accomplish and get parity with our line commenting system for text diffs in a merge request.

As for commenting on an image in the context of a commit, we can work on later as we figure out the text-based file browsing / file review use cases first, i.e. https://gitlab.com/gitlab-org/gitlab-ce/issues/25066

Added ~874211 label

As I understand it, a merge request diff is a diff between two branches, and we store any comments on the diff itself (and they are not associated with commits, etc.) Is this accurate?

@victorwu Without going into the database level details, that is accurate.

If so, would commenting on the merge request diff of two branches on an image be straightforward? I.e., if we wanted to comment on a particular spot in the image diff, just store the (x,y) coordinates?

Creating the image diff comment would be relatively straightforward, if we store {old/new},X,Y, specifying the side of the diff the comment is on, plus the X,Y coordinate. Would X,Y be enough though? Wouldn't we want to select a region in the image?

If someone updates the MR, we would see if the image was changed in that update. If so, we mark the discussion as outdated, and it will only be displayed on the Discussion tab, and no longer on the Changes tab (the same happens for line comments when the line is changed). If the image wasn't changed in the MR update, we can leave the discussion on the Changes tab (the same thing happens for line comments when the line isn't changed). Note that we can only detect if the image as a whole was changed, not if the specific X,Y in the image that the comment was on was changed.

I think that rendering the multiple discussions that can existing around one image on the Changes tab would be the hardest part.

If so, would commenting on the merge request diff of two branches on an image be straightforward?

[...]

As for commenting on an image in the context of a commit, we can work on later as we figure out the text-based file browsing / file review use cases first, i.e. #25066 (closed)

I'm not sure I understand the relation of commenting on an image in a commit (a change from old to new), and commenting on a file (the current version), and why commenting on an an image in a commit would be harder than commenting on an image in a merge request diff? As far as I can see, it's the other way around, since commenting on an image in a commit doesn't need to care about updates to the MR.

@DouweM : Gotcha. Thanks for the clarifications. Updating the description with you have and when we schedule this, we can dig further into the details.

Mentioned in issue #25190 (closed)

Changed title: Comment on pictures inside a commit (images as assets) → View and add comments on previous commits in image file view

Added ~897282 label

Removed to schedule label

@victorwu We are really talking about 4 features here that are interconnected:

Allow commenting on a changed image on a commit
Allow commenting on a changed image on an MR (depends on 1, relatively easy once 1 is done, code base will be cleaner with 1 + 2 than with 1 alone)
View and add line comments in blob view (https://gitlab.com/gitlab-org/gitlab-ce/issues/25066)
View and add image comments in blob view (depends on 1 and 3)

I think we should build and schedule them in that order. 1 and 3 can potentially be built at the same time.

/cc @smcgivern

Side note: @dimitrieh I don't understand from the mockups how to see the image without annotations? Do I really have to look at a particular version of the file? This isn't a problem for text diffs because they can be displayed inline, without actually obscuring the code. The regions you've picked seem to be really unlikely places for comments to happen, given that they are the background. How would it look with the region covering the cat's face, for example?

@DouweM @victorwu I wonder if just allowing file-level comments would be a quicker, simpler approach that would get us 80% of the way there, before we start with this.

@smcgivern I mentioned https://gitlab.com/gitlab-org/gitlab-ce/issues/20390 to @victorwu before, but couldn't find the issue! That would definitely be a simpler approach to start. We would be able to comment on images in diffs, just not yet on specific areas of that image.

@DouweM the commenting inline within an image is the real soul of this issue, but could be seen as an iteration.

I don't understand from the mockups how to see the image without annotations?

@smcgivern yes there should be a toggle to hide inline annotation markers

mmh perhaps my examples images are a bit weird: the blue square is the actual change within the image.. not a selected region or something like that. the blue dot is the comment/annotation marker (these will be toggle able).

So to clarify: There are no selection regions (this could be a separate iteration), just dot markers.. which point somewhere within the image.

All of this works together with the already existing image diffing functionality: 2-up, swipe, onion skin. Meaning that with 2-up, you would see the existing image and the changed image next to each other with 1 marker pointing at the same location within both files.

does this make more sense :) ?

@dimitrieh aha, I get it! Although even in that case, the dots could cover the eyes (say), right?

@smcgivern yep it has not relation to the image.. the person decides where that marker will be! So we need indeed the toggle! and perhaps make them a bit translucent

mentioned in issue #25066 (closed)

mentioned in issue #25419 (closed)

mentioned in issue #25824 (closed)

mentioned in issue #26259 (closed)

notes regarding this issue from the Mexico summit are here: https://gitlab.com/gitlab-org/gitlab-ce/issues/24816#note_21709657

@victorwu as I see this is on the schedule for https://gitlab.com/gitlab-org/gitlab-ce/issues/26259 is there a way we can get a small first iteration in?

@dzaporozhets as spoken of in the meeting, the first iteration should be pretty doable correct?

@dimitrieh : That issue (and all those issues) are not Deliverable for 8.17. They are in the "Other issues not in scope" section. So if there is free capacity later, we might be able to get to it. Mainly this would require some BE work and BE is backed up this iteration on breaking API changes to get ready for 9.0.

@smcgivern : Am I right to assume this would require some BE work?

@victorwu yes, I don't think we're likely to get to it in 8.17.

mentioned in issue #27357 (closed)

as spoken of in the meeting, the first iteration should be pretty doable correct?

@dimitrieh yes, lets have first step small and fast

mentioned in issue #27647 (moved)

@dzaporozhets + @dimitrieh : #27647 (moved) FYI

moved to gitlab-ee#1768

mentioned in issue gitlab#6343

mentioned in merge request !14785

View and add comments on previous commits in image file view

Resources

Problem

Solution and design

Out of scope

Previous description

Update (Oct 19 2016)

Designs

Child items ...

Activity

Comment on pictures (in-picture-comments) in diff

Pictures inside a commit

Implementation

Pictures inside a comment

Admin message

Admin message

View and add comments on previous commits in image file view

Resources

Problem

Solution and design

Out of scope

Previous description

Update (Oct 19 2016)

Activity

Comment on pictures (in-picture-comments) in diff

Pictures inside a commit

Implementation

Pictures inside a comment