change import media dialog box #1666

erichake · 2017-02-19T11:15:11Z

I've changed import dialog box for costumes, backgrounds and sounds and I've created categories, like this :

Beside, this import dialog loads costumes and backgrounds as light thumbnails instead of large picture files.

I've also found out that on my mac, a large amount of sound files were not loading correctly (wav codecs not recognised in some browsers, for example for "A-trombone.wav" on my firefox and my chrome). So I've changed .wav to .mp3, and it also reduced the Sound folder size by 3Mo.

All of this could be seen here :

http://db-maths.nuxit.net/snap/snap.html

For costumes, backgrounds and sounds, the import dialog box now display categories. For costumes and background, it displays small thumbnails instead of large original files.

cycomachead · 2017-02-20T10:10:24Z

I think this is really helpful!

A few things:

We should eliminate the description: "", in the JSON files since it's not used. (And in the case of costumes, it's a decent amount of bytes...)
Why not organize "section" as a higher-level object?
{ 'section-name': [{}, {},...], 'section-name': [{}, {},...]}
Maybe you did consider that and decided against it, but this seems like it might help make it easier to reason about how files are organized.
In the future, I think this could be split into a couple smaller pieces, like separating the thumbnails out because it will make it easier to review, but this is fine. :)

erichake · 2017-02-20T12:43:37Z

Yes you're right : I've commited the whole thing at once but I shouldn't have to do it this way... I understand it's not easy for you to review : I'm really sorry about that !

For description key you're right, I will remove it...
For section key I thought of what you say, but I decided to do it another way because I wanted to give the ability (in the future) for one image or sound to be attached to more than one section. For example :

{ "name": "birthday", "fileName": "birthday.mp3", "section": "Music" }

could become in future :

{ "name": "birthday", "fileName": "birthday.mp3", "section": ["Music" , "Party"] }

jmoenig · 2017-02-20T16:51:26Z

Thank you, @erichake , for this really nice contribution! You've really caught on the Morphic way of doing things very quickly and thoroughly, I'm impressed. I really like these changes, especially the categories and the thumbnails, this feels a lot better and is much nicer to use. I'll definitely take a closer look at this pull-request with the intention of integrating it into the main Snap trunk. But I'm going to have to ask you for a little patience, since I'm mostly travelling this week. In the meantime you've already convinced Bernat and Michael :-)

cycomachead · 2017-02-20T19:13:44Z

gui.js

-            msg.destroy();
-            myself.popupMediaImportDialog(folderName, items);
+    myself.getURL(
+        folderName + "/catalog.json",


This should be:

this.getURL(this.resourceURL(folderName, 'catalog.json'), ...

cycomachead · 2017-02-20T19:14:22Z

gui.js

+    myself.getURL(
+        folderName + "/catalog.json",
+        function (txt) {
+            myself.popupMediaImportDialog(folderName, JSON.parse(txt));


I'll need to this about this - we have a getMediaList function or something similar.

cycomachead · 2017-02-20T19:22:51Z

gui.js

+            return element.name;
+        },
+        null,
+        function () {


We don't need this.

cycomachead · 2017-02-20T19:25:21Z

gui.js

+
+    var colect = {};
+    items.forEach(function (item) {
+        var section = (item.section === "") ? "other" : item.section;


I think we might to make the check more broad... Maybe isNil(item.section) || item.section === '' ?

Otherwise if someone leaves off a section key then the sections list would have a "null" section.

Actually, I think it'd be an undefined section...anyway I don't think we want that either. :)

erichake · 2017-02-20T20:21:51Z

Thank's for this review, I've made the changes :

I think the getMedialList function is now useless in the code (it was looking for the COSTUME, BACKGROUND or SOUNDS text file inside the folders)
new ListMorph() takes care about undefined params : you're right I didn't see it !
ok for letting the possibility to "forget" the section key in the json files

erichake · 2017-02-20T20:26:23Z

Oups sorry I said a wrong thing : I didn't see that getMedialList was used by getExamplesProjectList...

nathalierun · 2017-02-21T04:04:47Z

@erichake Thank you for this great contribution 👍

Would it be difficult to add a right button in the costumes area to import a costume from the media library ? idem for the sounds in the sound area ?

Because it was the first thing I tried to do when I looked for the import dialogue box for the costumes.

AlainBusser · 2017-02-21T06:36:17Z

One other thing which would be great: That it be possible to draw the costumes with blocks, like draw a star with a sprite, then import the star as a costume; or draw a fraction with LaTeX and have it as a costume afterwards.

The biggest fun would be a "build a costume" block ;-) (imagine that a sprite could build its own costume when created as a clone)

jmoenig · 2017-02-21T06:57:33Z

@AlainBusser we have that in Snap! It's a library called save and restore picture drawn by pen

here are a bunch of sample projects that make use of this feature:

Valentine
Ferris Wheel
Towers of Hanoi

and the coolest of them all, the world's fastest fractals, by Dan Garcia:

Fractals

Enjoy!

cycomachead · 2017-02-21T10:04:37Z

I think the getMedialList function is now useless in the code (it was looking for the COSTUME, BACKGROUND or SOUNDS text file inside the folders)

I think this should be used, even if it's just calling JSON.parse.

But also I think we should extract the sections stuff out of the actual morph layout code. I'll write more later after work when I can better look at some stuff.

The mediaList could be adapted to look for folder/catalog.json which could also be used for libraries and example projects so that we don't have multiple formats.

For section key I thought of what you say, but I decided to do it another way because I wanted to give the ability (in the future) for one image or sound to be attached to more than one section. For example :

And this makes sense. Both formats are easy so I don't mind much either way, I was more curious as to why. :)

AlainBusser · 2017-02-21T10:19:05Z

Thanks for the examples; I have an other idea yet: Gerbert abacus

The sprites (Gerbert called them "apices") each have a numerical value (but depending on where they are inside the abacus) and each apice should have as a costume, the digit yielding its value.

I'll try it when I have some time

cycomachead

I left some code comments - most are minor.

This basically is 2 parts, including #1594
I think it would be good, if we do this to also update the code for libraries and examples, which would have no section data. It might make sense to do that first, but it doesn't matter me to. :)

cycomachead · 2017-02-22T09:32:42Z

index.php

@@ -0,0 +1,17 @@
+<!DOCTYPE html>


I don't think we want to commit this.

Oups sorry, I didn't see it was in my commit !

cycomachead · 2017-02-22T09:41:43Z

gui.js

+
+    listField.action = function (elt) {
+        msg = myself.showMessage('Opening ' + elt.name + '...');
+        // Not proud of this... I wasn't able to display the msg dialog


I think you're missing a call to .fixLayout() or popUp(). You can look at this, perhaps:
https://github.com/jmoenig/Snap--Build-Your-Own-Blocks/blob/master/gui.js#L6576

Hmm, there's possibly an issue with using the count variable. Not sure.

cycomachead · 2017-02-22T09:42:32Z

gui.js

+    };
+
+    listField.action = function (elt) {
+        msg = myself.showMessage('Opening ' + elt.name + '...');


If we're combining fields, then explicitly call localize() around opening.

cycomachead · 2017-02-22T09:46:11Z

gui.js

+            count = items.length;
+            items.forEach(function (item) {
+
+                if ((item.section === section) || (section === "All")) {


I think this function if much easier to read if it has:
if (condition) {return; } as the first statement and everything else is de-indented.

cycomachead · 2017-02-22T09:47:44Z

gui.js

+    sections.sort(sectionsSortFilter);
+    sections.push({ "name": "All" });
+
+    var listField = new ListMorph(sections,


This parameter should be on it's own line.

cycomachead · 2017-02-22T09:50:30Z

gui.js


+    var colect = {};


I think it might be good to put this in getMediaList, and have that function return something that has sections and the data. Otherwise I think it should be it's own small function, to get sections and sort items.

cycomachead · 2017-02-22T09:51:34Z

gui.js

+
+    content.color = dialog.color;
+    listField.setWidth(listFieldWidth);
+    listField.contents.children[0].alpha = 0;


Jens would know better, if there's no other way, but generally accessing another morph through children is not desired.

You're right : childrens are changing sometimes :-)
So I finally found the MenuMorph without dealing with children :

listField.listContents.alpha = 0;

is far better...

cycomachead · 2017-02-22T09:52:29Z

gui.js

-            );
+    var decrement = function () {
+        count--;
+        // console.log(count);


These should be removed.

cycomachead · 2017-02-22T09:53:35Z

gui.js

+            var section = (elt.name === "other") ? "" : elt.name;
+            frame.contents.children.splice(0, frame.contents.children.length);
+            count = items.length;
+            items.forEach(function (item) {


it probably makes sense to extract this into it's own function, so we can do items.forEach(renderIcon) or something.

cycomachead · 2017-02-22T09:54:09Z

gui.js

+                        };
+                        img.src = url;
+                    }
+                } else decrement();


There should be braces around all conditional expressions.

erichake · 2017-02-22T16:55:36Z

I've made the changes on last commit, but the setTimout is still in code of gui.js :

And I really don't like setTimeouts :-)...

brianharvey · 2017-03-18T06:06:13Z

I have a minor request: Could you change the little "SVG" flag to "vector" or maybe just "vec"? I would like our users not to have to know what an svg is. (And no doubt someday someone will invent another vector file format.) Otherwise it's great!

Adding "blessed by Jens" label because he did!

jmoenig · 2017-07-28T09:53:55Z

@brianharvey , you can't just add "blessed by jens". I really love this PR and we're going to pull it, but "blessed by jens" means that I've reviews the code and it's something that helps me remember I actually did. So, please don't confuse my memory!

That said, @erichake, this is beautiful, and now I did review it. :-)

jmoenig · 2017-07-28T10:01:27Z

Okay, sorry, I had to "unbless" this for now, because we found out, that a vast majority of the costumes are still broken, especially having the wrong size. This issue has been reported elsewhere in schools and needs to be addressed before we can pull this and work on it.

@brianharvey , what's the status on the broken costumes? These need to be fixed urgently, so we can work on both this and on the release.

jmoenig · 2017-07-28T10:01:54Z

That said, @erichake your changes are beautiful, and we will incorporate them!

brianharvey · 2017-07-30T19:26:48Z

Sorry, Jens, I thought I'd understood you to say that it was blessed.

Remind me what's wrong with the costumes? I just copied them from Scratch.

jmoenig · 2017-07-30T20:56:39Z

@brianharvey , many costumes are way too big, about double the size they are in Scratch. Teachers (and Mary) have complained about that a lot over on Piazza, because they have to tell students to scale the whole sprite down to 50% size to get them to look right. Worse, some costumes are "normal", and others are not, so changing to another costume often requires a sprite-resize.

I'm pretty sure that this is an unintentional artifact that happens when you edit graphics on a retina display. Not in Snap but in some other image processing app. Anyway, this has been pretty urgent for a longish time, so we should work on it now, to fix it for the upcoming release. After that I really want most of @erichake's contributions. If we can get the costumes right in time, I'd love to include these contributions in the upcoming release as well.

brianharvey · 2017-07-30T21:23:42Z

Hmm. It looks to me as if the problem is with new Scratch-2.0 bitmap costumes; vector costumes and old Scratch costumes both seem okay. I didn't edit any of the costumes, though, so I don't believe your theory. My theory is that Scratch added double-density costumes on purpose for the benefit of retina displays, and they have code that half-sizes them on input on non-retina displays. Will investigate further...

cycomachead · 2019-08-05T07:08:42Z

We really should work on this -- not sure how big the conflicts are now, but it should be fairly easily resolved.

cycomachead · 2020-01-12T22:35:02Z

@jmoenig @brianharvey How do we feel about tidying this up and merging soon?

The biggest "downside" is having thumbnails in the repo, but it's really not all that much data.

brianharvey · 2020-01-13T02:28:51Z

Yes, please!

change import media dialog box

2156306

For costumes, backgrounds and sounds, the import dialog box now display categories. For costumes and background, it displays small thumbnails instead of large original files.

erichake added 2 commits February 20, 2017 15:47

re-import alonzo.svg

1264208

delete description key in catalog.json files

0c59981

cycomachead reviewed Feb 20, 2017

View reviewed changes

gui.js Outdated

return element.name;

},

null,

function () {

Copy link

Collaborator

cycomachead Feb 20, 2017

Choose a reason for hiding this comment

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

We don't need this.

cycomachead reviewed Feb 20, 2017

View reviewed changes

changing few things

623ad6b

cycomachead reviewed Feb 22, 2017

View reviewed changes

adapt few things from review

0503db4

jguille2 mentioned this pull request Mar 16, 2017

Built-in Costume Sizing Issues #1682

Open

brianharvey added blessed by Jens ;-) IDE labels Mar 18, 2017

jmoenig added under review and removed blessed by Jens ;-) labels Jul 28, 2017

jguille2 mentioned this pull request Dec 29, 2017

Costumes by category #1975

Open

bromagosa mentioned this pull request Dec 18, 2018

Speed up open dialog by caching project list? #2291

Closed

change import media dialog box #1666

Are you sure you want to change the base?

change import media dialog box #1666

Uh oh!

Conversation

erichake commented Feb 19, 2017

Uh oh!

cycomachead commented Feb 20, 2017

Uh oh!

erichake commented Feb 20, 2017

Uh oh!

jmoenig commented Feb 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichake commented Feb 20, 2017

Uh oh!

erichake commented Feb 20, 2017

Uh oh!

nathalierun commented Feb 21, 2017

Uh oh!

AlainBusser commented Feb 21, 2017

Uh oh!

jmoenig commented Feb 21, 2017

Uh oh!

cycomachead commented Feb 21, 2017

Uh oh!

AlainBusser commented Feb 21, 2017

Uh oh!

cycomachead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichake commented Feb 22, 2017

Uh oh!

brianharvey commented Mar 18, 2017

Uh oh!

jmoenig commented Jul 28, 2017

Uh oh!

jmoenig commented Jul 28, 2017

Uh oh!

jmoenig commented Jul 28, 2017

Uh oh!

brianharvey commented Jul 30, 2017

Uh oh!

jmoenig commented Jul 30, 2017

Uh oh!