Often we're updating icons and documentation screenshots and such, and it would be great if reviewboard just displayed the images, so reviewers could evaluate those. In my case, there are git hashes, so reviewboard has a chance of getting the raw binary bytes. Thoughts?
We wouldn't really be able to make this work with Git the way you'd hope, as Review Board wouldn't necessarily have access to the modifications. Today, you can upload screenshots (really, any image) much like you can with diffs and people can review that by clicking and dragging on the screenshot to comment on that region. Would that suffice?
The typical diff from git gives you something like: diff --git a/docs/sdk/htmltable.png b/docs/sdk/htmltable.png new file mode 100644 index 0000000..60617a2 Binary files /dev/null and b/docs/sdk/htmltable.png differ Seems like there's enough there that reviewboard could ask git for the blob: $git-cat-file blob 60617a2 > /tmp/z; file /tmp/z /tmp/z: PNG image data, 813 x 142, 8-bit/color RGB, non-interlaced I've noticed that reviewboard already does cat-file (our reviewboard server polls git every 5 minutes, and sometimes refuses to accept patches against versions that don't yet exist), so seems like it could do the same here? Am I missing something? Thanks!
We can only get the original, not the modifications. There's no way to see what changed, which makes it totally useless. There's also no way of making it work with other types of repositories. What we'd have to do is provide support in post-review for grabbing these modified binary files, figuring out the correct mimetypes, and uploading them. That might happen in a future release, but is not as easy as it may sound.
Seems fair to relegate this sort of feature to post-review. For images used in web development, just seeing old and new (both of which may be accessible using git-cat-file, if using git) is often good enough, and that's probably the right thing to upload.
- NeedInfo + New
I don't see why you can't get the uploaded patch-file to include the binary diff. <Edit image> # hg diff test.png --text > my-review.patch # hg revert test.png # file test.png test.png: PNG image, 310 x 320, 8-bit/color RGBA, non-interlaced # patch -p1 < my-review.patch patching file test.png # file test.png test.png: PNG image, 320 x 281, 8-bit/color RGB, non-interlaced Seems to me that reviewboard will have all the info it needs.
I've found this ticket by searching for a way to review the image diffs which, unfortunately, still doesn't seem possible even with the latest version and I admit I don't understand what exactly the problem with getting the old and new contents of the file is. At least with git,
git diff --binarycan be used to include the full image contents in the diff, so the data could definitely be provided to ReviewBoard.
It seems really a pity that there is already an image review tool, but it can be only used for the attachments and not the files included in the review.
Hello! It's Juan!
This would be super helpful for us GUI/design guys.
Review Board does do diffs of images that are attached to review requests, which is probably more what you want (since I assume you're not actually committing images to version control?)
See https://www.reviewboard.org/docs/manual/2.5/users/reviews/reviewing-images/ and https://www.reviewboard.org/docs/manual/2.5/users/reviews/reviewing-files/#reviewing-file-attachments
Actually, we do check into version control the icons and other images that show up on the final product.
The diffs/comments on attached images are super useful and we attach screenshots on every GUI review. Would be great to do something similar to committed images too.
There's a lot of work that went into the tree for allowing review of images and other file types in diffs, but we never put together all the actual support for uploading files (as they're not contained within diffs themselves, except on Git in a way that's not efficient enough for our usage). I want to get that into 4.0 (3.0 is currently under development and we're working to finish off the last of the features).
If there is no way to get the images automatically from the diff/repository, there will be little difference to attaching manually the new image files to the review (like we do today).
It will fetch from the repository and from your local change. That's the part that's just not written yet, and the reason we haven't made the rest of it public yet. It will require uploading via RBTools, though.