An Unknown Object!

◀ Merged tasks: T2705.

How did you get the Unknown Object attached? (ie did you attach from Pholio or Maniphest and what did you intend to attach...)

I clicked Edit Pholio Mocks and it was listed, so I attached it.

re: UNKNOWN OBJECT - the sort of good news here is it looks like we just have some bum objects in the production install? Not sure what's happening exactly.

What parts of M44 are ready to be implemented?

It should be all clear, I'll look over it and see if there were any lingering questions. It's probably easy to split up starting with:

• Replace lightbox controls with the new 'bottom bar' controls.
• Add new image brower.
• Add new inline image commenting.
• Update Pholio Mock page (i'll probably take another deep look here this week)

The one thing I want to get a better handle on here is how the draft -> submit -> email/notify workflow works, especially if these controls come to lightboxes. In particular:

• When you "save" an image inline, what state does it go into? Elsewhere, inlines go into a "draft" state and are submitted collectively on an object.
• If there's a draft state, how do you submit the comments?
• What emails do we send, and to whom, and when?
• What do the emails say?

For example, here's a scenario:

• I open up M123.
• In the comments, @btrahan has written {F123} in an older version of comment, which he then edited.
• I click it and bring up the lightbox.
• I leave inline comments about it.
• (Possibly, I submit a mock comment to finalize them.)

Now:

• Who gets emailed? What are the contents of the email? Who gets notified? What does the notification say?
• If I navigate to F123, what do I see?

(I don't have good answers for the majority of those questions; all ways forward that I see are fraught with confusion.)

I thought we agreed to not attach the comments to the files, but keep them on the object being viewed, ie task, mock, diff.

(somewhere in IRC)

Oh man, I missed this IRC bit.

So from a data perspective, Pholio has PholioImage which is a wrapper on PhabricatorFile.

At the end of the day, this is unified PhabricatorFile commenting interface, where the only PhabricatorFiles involved are one that are viewable images.

My suggestion is thus to actually collapse PholioImage -> PhabricatorFile. Any / all comments through this interface go the same place - the actual PhabricatorFile. However, Pholio surfaces the Files along with their comments in a fancy UI.

I'll defer to Chad on how things are submitted, etc EXCEPT I assumed once in this fancy UI you have to submit while on any given image / set of images. Drafts and whatnot could still exists, but only in the context of what you're looking at, and if you try to navigate away you get a warning that you didn't submit your data yet and will lose it if you go.

Notifications are interesting... If the PhabricatorFile is attached to content in another application, notifications should go out through that application. Additionally, I think notifications should be sent out for PhabricatorFile stuff directly.

So for everyone's scenario --

• notifications are sent out to the creator of F123 and any other subscribers to it
• no one is notified who is just involved with the mock since this file was just something mentioned in the mock and not explicitly attached as a "mock file"
• the notification says something that is Phabricator File specific
• when you go to F123 you see the comment

"So for everyone's scenario" => "So for Evan's scenario"

There's all sorts of similarly terrible typos in the earlier comment; sorry. :/

Let me take another stab...

I think its most clear if the following is true:

• pholio has its own super special UI. it is like the lightbox UI but not as ajaxy. Most importantly, you get to submit comments across multiple images.
  • these comments are surfaced on a given mock _and_ on a given file
  • we should indicate on the file info page that the file is attached to a mock
  • we should overall position our tools such that if you need to do nitty, gritty image feedback you should use pholio
• nothing else has special UI -- we're just upgrading the lightbox
  • lightbox now lets you submit as many comments on the current image as you want!
    • you have to submit while viewing that image.
    • you get a warning if you try to navigate away that you will lose your comment(s)
  • an image (which is a special file) has a contiguous set of comments across applications
• I think Differential / Diffusion should stay mostly the same i.e. viewable images in comments and whatnot get the special lightbox UI, but files in the actual changeset do not get any new special UI.
  • we could also get away with a new action to "View in lightbox" UI or what have you. I think that could be done clearly / cleanly _maybe_ and then those comments should dual post like in the pholio case.

Yeah in the beginning I wanted comments to live on files, but that model is both difficult and not very useful. Product wise I think comments should continue to live on the object, which that transaction references an x/y point on an image file. If a file exists in two places, those comments don't show up on the other (helps with privacy).

Essentially, we expect people to have long, referenced, conversations in Pholio - then embed/attach the mock set for reference elsewhere. Light conversations can happen on images attached in other places (Conpherence, Differential, Maniphest). I find that also very useful and means I'm more likely to get feedback in small bits.

I also feel most Files never get re-used anyway. If they are, then they should be Macros or Mocks.

@chad - how would you answer epriestley's scenario?

Also, one complication I want to make sure is clear is that there are images attached to the mock and then a reference to yet another file in the comments (this F123). Right now such a mention doesn't make F123 an image of the mock but it does invoke the lightbox UI.

Filed T3742 to discuss my crazier ideas around Files... :D

(T3651 might not have been important depending on how this pans out. If we want comments to be per object then I think Application Transactions should have the notion of "Image Comment" so we can reduce / reuse code for anything that implements application transactions.)

Maybe I'm doing a bad job of explaining what I expect over email, if this doesn't make sense we can Google Hangout on it. 8)

Basically I don't want commenting on Files, I want commenting on (PHID+FILEID). So everything is preserved in the context of a parent object (commenting, notifications, etc). If I comment on F123 on a task, and F123 is also in a diff, the comment is only on the task, not the file permalink, not the diff. Only people on the task are notified.

But I don't know how the backend works if this suggestion is totally unscalable or something. It seems like it solves Evan's concerns?

I follow that. (See https://secure.phabricator.com/T3612#comment-23 for my vague technical aside on how to implement that version.)

Some specific questions from Evan that are still outstanding to me:

• What does the email / notification say? (I'd add should there be different text for this commenting on F123 versus the text for commenting on a properly attached image?)
• If I navigate to F123, what do I see? (I hear you've said nothing Chad :) but do you really mean nothing? Big technical differences in implementation. If its about getting feedback in small pieces all over, shouldn't it aggregate somewhere for that file?)

The part that seems strange to me from a product perspective is when you mention other things on an object it doesn't currently start a (phid, other thing id) private conversation so much as link to the other thing so discussion is aggregated on other thing. I generally like openness by default (so some sort of union of who gets to see and comment on the file as things are shared on various objects) and consistency (though I admit I do this to a fault as my definition of consistency can be weird.) I could also see arguing that private conversations should be spawned by these sorts of mentions by default.

Otherwise on product thoughts, see T3742 but I think we also have an opportunity to make Files themselves more powerful. I'd generally like to make sure we think about this a bit as I don't think the implementation gets longer - albeit maybe delayed - and we save ourselves oodles of re-implementation time down the road.

Also, I've always thought it was a bit strange that mentioning a file dumps it into the lightbox set with attached files in say Maniphest as these seem silly and important to me, respectively.

Anyway, yeah, lets google hangout if this isn't making sense. I'll even write down whatever we come up with. :D

• I'm assuming we'd just keep notifications / emails the same as they are now with inline image comments, they seem fine to me.
• I don't see any need to store any comment info on F123 page directly. Comments without the full conversation context would be difficult to follow and it's not something I'd expect people to go looking for. If anything we could point to objects the file has been added to,.

I think we're actually talking about comments on comments here, with a special case being comments on (files in) comments. Or, really, comments on remarkup blocks.

In particular, if I reference F123 several times on the same object (e.g., post several comments containing {F123}), I think the expectation is that the inlines are bound to a specific instance of that file, and don't appear on everyone's {F123} in the whole thread. Similarly, if I make a comment which is just {F123} several times in a row, I think the expectation is that inlines are bound to the image I actually click on, not all duplicates of that image in the comment (or, generally, associated with the object). (A possible exception / special case, which might be quite difficult, is that if I edit a comment and the former version of the comment had an image with inlines, the inlines probably are expected to carry forward.)

So some of the issues we need to figure out are:

• How do we assign an ID/PHID to any arbitrary block of remarkup? (For example, the summary and test plan in Differential do not have comment PHIDs now and still won't after ApplicationTransactions).
• How do we address/identify individual instances of file embeds within remarkup (e.g., the first {F123} vs the second {F123} in a comment)?

If we sort those out, we can then add "CommentComment" transactions (or similar), which are like inline comments but have a remarkupBlockPHID and then some additional data specifying where in the block the comment is (in theory, this could also support inline comments on comments, and threads/replies to comments) instead of, e.g., oldLine/newLine.

I think we run into some hard cases with editing. For example, if I embed {F123} in a revision summary, and Bob leaves an inline comment with {F124}, and Chad leaves an inline comment on that image with {F125}, and I leave an inline comment on that image with {F126}, and then I edit the summary to remove {F123}, how do we get to my four-levels-deep inline? It seems bad to completely orphan it; it probably has to be something like: find the edit transaction, click show details, click some button to render the things as remarkup (?), click the original {F123}?

All applications need to become URI-aware too, so we can generate links to inline comments (potentially with arbitrarily deep nesting).

I don't see any ways to do it more cheaply or easily. The best idea I have is:

• Provide some way to quote an image and some kind of UI in Remarkup for editing? So your comment would actually have {F123, comment1="this should have more color", pos1=23;35;300;400} or something in it, which would mean "embed a copy of F123 with a comment on it at such-and-such position". This requires no database/backend/email/notification changes, but it seems cumbersome to build a reasonable UI for and the end result is kind of yuck.

Broadly, I'm concerned that this is like months of work in terms of implementation cost and increased complexity in other systems (we still haven't managed to bring ApplicationTransactions to Differential and Maniphest, even without this) and we end up with a feature which is neat but is basically the end of a development line (it isn't an infrastructure investment which unlocks other features) and probably sees only limited use (today, you can just leave a comment, like "that mock looks good but the buttons should be greener" or something, which I think is what we do and doesn't seem too onerous and doesn't happen too often).

I think T1460 hasn't been mentioned, but probably interacts here and further increases complexity (what reply/done states to comments-on-comments have?).

I see no reason to push this feature if it's that significant. It does seem like it needs more coordination between all of us to get a decent game plan and even then it's probably not worth the time investment given the current roadmap.

I do think long term there is value here, especially even if its just commenting on images in Pholio and Differential. There are certainly times Evan posts a screenshot up with the test plan (awesomely helpful) and I want to highlight something and say "make sure this is 12px".

So as long as we maybe keep this end goal is in mind as we scope out other systems, I'm perfectly happy.

I am back from DMV and happy to Google Hangout if people want today. I have a short meeting at 1, but am free until 5 after.

I'm happy to jump on a Hangout -- maybe at 3PM?

3 PM it is!

Doesn't look possible to do "Image x of y" in current phtize from JS... Worth adding or just changing the language? I don't have any great ideas on updating the language. Unsure x of y is useful though.

I'd say just get rid of it, or render "X / Y" without pht()?

(To answer your question more directly: yeah, no way to do pht() with variables in Javascript. We can build that without TOO much trouble, but I'd like to wait for a really good use case first.)

Woo came here from the blog. Just a few notes so far:

1. The lightbox on the blog doesn't seem to show anything under the Comment pane even though I should be logged in (to admin.phacility.com). Or is the blog.phacility.com completely isolated/separate? These were errors I got in Firefox console, which I don't get when using lightbox on this instance.
   • Error: JX.$('phabricator-help-menu') call matched no nodes.
   • XHR POST https://blog.phacility.com/file/thread/PHID-FILE-3ittlvgojn2326tbg22z/ [HTTP/1.1 404 Not Found 338ms]
2. I'm very used to "click anywhere that isn't the image to close the lightbox" behavior. My personal preference would be to maintain that when clicking on any empty space outside the image/file. I think this is due to where my mouse is located when I click the image (towards center of screen usually) and where I would go to click to close (in most cases just slightly down vs. top right corner). But then I use a trackball so...
3. Is PDF preview considered? PDFs were specifically mentioned in the blog so I have to ask~
4. (Maybe not totally related) Some way to easily reference the same file instance from different areas. Most users I see will continue to drag/drop the same file in different areas and not use the same file reference object {F1234} - which I'm assuming would mean two separate comment chains, etc. Maybe somewhere in the lightbox listing the tasks/diffs/etc. where the file is referenced would help understanding. Or a way to view/copy the file remarkup to be used elsewhere. Or maybe this is all T11543.

Overall I think it looks great! I'm especially excited about the new file rendering in remarkup!

• I don't think that's a lightbox bug, just a general blog bug.
• To be fixed.
• If it's native? We won't be adding third party code for this.
• We added F1234 to the header of the lightbox for resharing.

Ah I totally missed the F1234 being in the lightbox already. And I did figure that using 3rd party code to render PDF would be out of the question. I don't really know what the current state of PDFs in browsers is so I had my project manager hat on for that question~

https://mozilla.github.io/pdf.js/