Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Generate, upload, and display thumbnail of shots. #3712

Merged
merged 1 commit into from
Nov 27, 2017

Conversation

chenba
Copy link
Collaborator

@chenba chenba commented Nov 1, 2017

  • The thumbnails are generated on the browser.
  • The thumbnails are scaled and cropped in the same fashion as how My Shots display shots as thumbnails currently; they are not simply scaled down copies of the shot image.
  • The thumbnails are always PNGs.
  • This patch piggyback on some existing thumbnail related code (that did not seem to be in use?). That code inserts the thumbnails into the images table and allows the thumbnails to be managed identically to the shot images.
  • My Shot will fall back to using the full size shot image if a thumbnail isn't available.

Fix #3282

@chenba
Copy link
Collaborator Author

chenba commented Nov 20, 2017

In the patch for #3728 (PR #3819), I made some changes on how the shots are displayed as thumbnails. I will also simplify how this patch scale and crop the shots to generate thumbnails; it's a bit overwrought at the moment.

[WIP] will be in the title of this PR until that is ready.

@chenba chenba changed the title Generate, upload, and display thumbnail of shots. [WIP] Generate, upload, and display thumbnail of shots. Nov 20, 2017
@chenba chenba changed the title [WIP] Generate, upload, and display thumbnail of shots. Generate, upload, and display thumbnail of shots. Nov 23, 2017
Copy link
Contributor

@ianb ianb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a couple small notes. There's a TODO as well: is that just a leftover?

Commits should get squashed before going to master.

@@ -33,8 +34,9 @@ this.takeshot = (function() {
}
let convertBlobPromise = Promise.resolve();
if (buildSettings.uploadBinary && !imageBlob) {
imageBlob = blobConverters.dataUrlToBlob(shot.getClip(shot.clipNames()[0]).image.url);
shot.getClip(shot.clipNames()[0]).image.url = "";
let clipImage = shot.getClip(shot.clipNames()[0]).image
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: semicolon

let headers;
// @TODO: add thumbnail to payload
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still a todo?

});
return fetch(req).then((resp) => {
if (!resp.ok) {
var errorMessage = "Error saving edited shot";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: let (and above)

app.put("/data/:id/:domain", upload.single('blob'), function(req, res) {
app.put("/data/:id/:domain",
upload.fields([{name: "blob", maxCount: 1}, {name: "thumbnail", maxCount: 1}]),
function(req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the indentation below should be changed

`).split(/\s+/g);

// Attributes that will be accepted in the constructor, but ignored/dropped
AbstractShot.prototype.DEPRECATED_ATTRS = (`
microdata history ogTitle createdDevice head body htmlAttrs bodyAttrs headAttrs
readable hashtags comments showPage isPublic resources deviceId url
fullScreenThumbnail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize we had never deprecated fullScreenThumbnail (which was from when we were doing more expansive data capturing). Anyway, good to have it gone.

@chenba chenba merged commit 6bafd7d into mozilla-services:master Nov 27, 2017
@chenba
Copy link
Collaborator Author

chenba commented Nov 27, 2017

Yep, that TODO was a leftover I missed. Squashed and merged.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants