Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support annotating image attachments #137

Closed
michaelcfanning opened this issue Mar 30, 2018 · 18 comments
Closed

Support annotating image attachments #137

michaelcfanning opened this issue Mar 30, 2018 · 18 comments

Comments

@michaelcfanning
Copy link
Contributor

michaelcfanning commented Mar 30, 2018

We have attachments today. A recent SARIF evaluator has noted that a screenshot which is associated with a result often comes with a bounding rectangle that highlights some area of interest in the screenshot. This bounding rectangle is the equivalent of a region associated with a source file. Probably this should be an array of bounding rectangles.

We might consider adding a region and/or set of annotations as well, so that other kinds of attachments could be associated with selected internal areas.

@ghost
Copy link

ghost commented Mar 30, 2018

This is fascinating.

@michaelcfanning
Copy link
Contributor Author

@lgolding Updated title with actual suggestion. I have more offline details on this SARIF producer I can provide.

@michaelcfanning
Copy link
Contributor Author

@lgolding request this for CSD1 as we have a committed SARIF v2 producer who has requested the feature

@ghost ghost self-assigned this Apr 14, 2018
@ghost ghost added the CSD.1 Will be fixed in CSD.1. label Apr 14, 2018
@ghost
Copy link

ghost commented Apr 14, 2018

@michaelcfanning Yes, I'll do it in CSD.1 and write it this weekend.

@ghost
Copy link

ghost commented Apr 17, 2018

@michaelcfanning Yes, please send along details of the SARIF producer.

@ghost
Copy link

ghost commented Apr 20, 2018

Proposal: Add regionAnnotations to attachment. Add screenshots to result. Invent supporting objects as shown.

attachment
  =description:message
  =fileLocation
  +regionAnnotations:regionAnnotation[] 

result
  =attachments:attachment[]
  +screenshots:screenshot[]

+regionAnnotation
  region
  message
 
+screenshot
  description:message
  fileLocation
  areaAnnotations:areaAnnotation[]
 
+areaAnnotation
  rectangle
  message
 
+rectangle
  top:integer, left:integer, bottom:integer, right:integer

@ghost
Copy link

ghost commented Apr 20, 2018

@michaelcfanning Said ok in email.

You know, there's an alternative: Give region a message property. Then any region can be "annotated", the regionAnnotation object in my proposal goes away, and we have instead:

attachment
  =description:message
  =fileLocation
  +regions:region[] 

result
  =attachments:attachment[]
  +screenshots:screenshot[]

region
  +message
 
+screenshot
  description:message
  fileLocation
  areaAnnotations:areaAnnotation[]
 
+areaAnnotation
  rectangle
  message
 
+rectangle
  top:integer, left:integer, bottom:integer, right:integer

@ghost
Copy link

ghost commented Apr 23, 2018

@michaelcfanning Even better (IMO): give rectangle a message and dispense with areaAnnotation:

attachment
  =description:message
  =fileLocation
  +regions:region[] 

result
  =attachments:attachment[]
  +screenshots:screenshot[]

region
  +message
 
+screenshot
  description:message
  fileLocation
  recangles:rectangle[]
 
+rectangle
  top:integer, left:integer, bottom:integer, right:integer
  message

We move towards a design philosophy where many objects have messages, just like many objects have property bags.

@michaelcfanning
Copy link
Contributor Author

i am concerned about adding message to region. why aren't we utilizing the annotations objects here? the encapsulate a physical location, which can encapsulate regions. in this use, only the region information of the PLC may be populated.

also, what if we allowed bounding rectangles for attachments, just noting that they are only relevant if the attachment is an image? Leaving the following reduced proposal:

attachment
=description:message
=fileLocation
+annotations
+recangles:rectangle[]

result
=attachments:attachment[]

+rectangle
top:integer, left:integer, bottom:integer, right:integer
message

@ghost
Copy link

ghost commented Apr 23, 2018

I like your proposal. I didn't use annotation because all the annotations on a screenshot referred to the same file. I didn't want to verify that all the fileLocations in the annotations referred to the same file.

But your way is simpler. I'll send it out. This eliminates the screenshot value for role, which I also like. Not every image of interest is a screenshot, but any image might benefit from annotating like this.

@ghost
Copy link

ghost commented Apr 23, 2018

@michaelcfanning I'm still not wild about using the annotation object when you already know the fileLocation and just need the region. Yes, there are one or two other places in the spec where we say things like "you have to mention the URL unless you're inside a file object that already mentions the URL" but I've never liked it.

What's your objection to region.message?

It would be symmetric with rectangle holding its own message, and it would make attachment symmetric between regions and rectangles:

attachment
  =description:message
  =fileLocation
  +regions:region[]
  +rectangles:rectangle[]

@michaelcfanning
Copy link
Contributor Author

@lgolding: why does physical location need a message if region has a message?

@ghost
Copy link

ghost commented Apr 23, 2018

@michaelcfanning Not every physicalLocation has a region. Some just have a fileLocation.

@michaelcfanning
Copy link
Contributor Author

what's the distinction between physicalLocation.message and physicalLocation.region.message if a viewer finds that both have been populated?

@ghost
Copy link

ghost commented Apr 23, 2018

It could use region.message as hover-text (which, remember, was the original motivation for annotation).

@ghost
Copy link

ghost commented Apr 23, 2018

It's the same use case we imagine for rectangle.message, right?

@michaelcfanning
Copy link
Contributor Author

ok, i think i buy it.

@ghost ghost changed the title Consider adding an explicit screenshots object Define an explicit screenshots object Apr 24, 2018
@ghost ghost changed the title Define an explicit screenshots object Support annotating image attachments Apr 24, 2018
@ghost
Copy link

ghost commented Apr 24, 2018

@michaelcfanning Retitled to match the design we settled on.

@ghost ghost added the design-improvement label Apr 24, 2018
ghost pushed a commit that referenced this issue Apr 24, 2018
ghost pushed a commit that referenced this issue Apr 24, 2018
ghost pushed a commit that referenced this issue May 3, 2018
@ghost ghost added the resolved-fixed label May 3, 2018
@ghost ghost removed the change-draft-available label May 3, 2018
ghost pushed a commit that referenced this issue May 3, 2018
@ghost ghost closed this as completed May 3, 2018
ghost pushed a commit that referenced this issue May 4, 2018
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant