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

New addon: sort assets by alphabetical order #7247

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

Jazza-231
Copy link
Contributor

Resolves suggestion from the community server

Changes

Adds an option to the edit menu to sort the assets by alphabetical order.

Reason for changes

In a future version of this when I make it, it could be used to fix the order of broken gifs? will add settings for different sorting methods, current is basic js array sort.

Tests

works on brave for both costumes and sounds, sprites are not done yet, and i need pointers on how to go about that. if its not feasible, sprites will be ommited (they arent that useful to order anyway)

@Jazza-231
Copy link
Contributor Author

Alphabetical
Current implementation allows costumes and sounds to be sorted using .sort()

@Jazza-231
Copy link
Contributor Author

I have implemented a better sorting system, which uses natural sort. this is just...better, but it is slower, and when a lot of things are sorting, it can take a while. I am not sure if this can be improved. besides that, sprites still need to be done, and for consistency i would like to find a way to make the menu close when the item is clicked on while it is greyed out (which you will see when you click on the grayed out "restore" button) i am not sure how to do either of those, and believe me i have tried. if you have ideas, please tell me, and if you think those things are fine as is, i guess no changes to them need to be made

@Jazza-231
Copy link
Contributor Author

2024-03-03.19-44-27.mp4

oh, and anyone know why it doesnt come back when the addon is reenabled while the menu is open and it was disabled before the menu was opened, but it does come back when it was enabled before the menu was opened, disabled when the menu was open, and reenabled when the menu was still open?

@Joeclinton1
Copy link
Contributor

Joeclinton1 commented Mar 3, 2024

For sprite sorting a function exists similar to reorderSound or reorderCostume, it's called reorderTarget.

@Jazza-231
Copy link
Contributor Author

For sprite sorting a function exists similar to reorderSound or reorderCostume, it's called reorderTarget.

oh my god thank you how did i not see that

@Jazza-231
Copy link
Contributor Author

(latest commit does not work)

@Jazza-231
Copy link
Contributor Author

Jazza-231 commented Mar 3, 2024

ok im pretty sure it works, sprites are done, the performance is actually really good, now just need to figure out how to close that menu

@Joeclinton1
Copy link
Contributor

Chatgpt thinks your natural sort can be done by using localCompare, which would make it alot more concise. Could you test?

Array.prototype.naturalSort = function() {
    return this.sort((a, b) => 
        String(a).localeCompare(String(b), undefined, {numeric: true, sensitivity: 'base'})
    );
};

@Jazza-231
Copy link
Contributor Author

Chatgpt thinks your natural sort can be done by using localCompare, which would make it alot more concise. Could you test?

Array.prototype.naturalSort = function() {
    return this.sort((a, b) => 
        String(a).localeCompare(String(b), undefined, {numeric: true, sensitivity: 'base'})
    );
};

well ill be damned it seems to sort the same

@Joeclinton1
Copy link
Contributor

Joeclinton1 commented Mar 3, 2024

ok im pretty sure it works, sprites are done, the performance is actually really good, now just need to figure out how to close that menu

See here

You need to get the menu-bar and call this.props.onRequestCloseEdit()

Edit: if you look at what onRequestCloseEdit calls, you can probably close the menu with a redux dispatch

@BlueGoat11

This comment was marked as spam.

@Jazza-231
Copy link
Contributor Author

ok im pretty sure it works, sprites are done, the performance is actually really good, now just need to figure out how to close that menu

See here

You need to get the menu-bar and call this.props.onRequestCloseEdit()

Edit: if you look at what onRequestCloseEdit calls, you can probably close the menu with a redux dispatch

How do you find this shit, I looked so hard...I can't do this tonight so remind me to do it tmr

@Jazza-231
Copy link
Contributor Author

I am happy to have figured out that redux event on my own, redux is one of my weaknesses so that felt good, anyway i think this is all done now!

@Jazza-231 Jazza-231 marked this pull request as ready for review March 4, 2024 03:05
@Samq64 Samq64 added type: enhancement New feature for the project new addon Related to new addons to this extension. `scope: addons` should still be added. scope: addon Related to one or multiple addons labels Mar 5, 2024
Copy link
Contributor

@Joeclinton1 Joeclinton1 left a comment

Choose a reason for hiding this comment

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

The actual reordering works perfectly for sprites, costumes and sounds in my tests. Edit menu works fine.

  1. Dynamic/enable works properly in all cases except the edge case that the menu is already open. Can you make it check if the menu is open when enabled and insert the option?
  2. The originally selected asset is no longer selected after sorting, can you fix this? Take a look at the code in New Addon: Asset Conflict Resolution Dialog #7232 for how to select an asset. You can probably copy and paste it. You'll need to do the sprites a little bit differently though, but it's really just a case of adding another selector.
  3. I would've preferred a context menu option over an edit menu option, but i guess since this is applied to all costumes instead of one it kind of makes sense for it to not need context. Having to go to the code tab to sort sprites is a little awkward but sprites are kind of an after thought anyway.
  4. I don't like the While True loop. Addons should not use while loops unless absolutely neccessary for performance reasons. There is an event triggered when that menu is opened why not just trigger your code on that? Otherwise the code is good.

@WorldLanguages
Copy link
Member

What's the expected behavior of Ctrl+Z/undo after sorting alphabetically?

@Jazza-231
Copy link
Contributor Author

What's the expected behavior of Ctrl+Z/undo after sorting alphabetically?

Well one would assume it will undo the reordering.. I'm not sure if this actually happens, I think all I need to do is call some function in the VM and that will save it as a snapshot that can be undone.

@DNin01
Copy link
Member

DNin01 commented Apr 9, 2024

What's the expected behavior of Ctrl+Z/undo after sorting alphabetically?

Are you talking about the undo buttons in the costume editor?

Ctrl+Z can't usually undo costume deletions, additions, or reorders, AFAIK.

@Jazza-231
Copy link
Contributor Author

Jazza-231 commented Apr 9, 2024

Ctrl+Z can't usually undo costume deletions, additions, or reorders, AFAIK.

That is correct, but perhaps in this case it should? Or maybe change the sort button to be restore previous order?

@WorldLanguages
Copy link
Member

Ctrl+Z can't usually undo costume deletions, additions, or reorders, AFAIK.

Well, I meant Edit > Restore Costume then.
Of course, Scratch won't let you undo reorders, because you can only do one reorder at a time, so it's trivial to undo it manually. But if we're going to do multiple reorders with a click of a button, it sounds sensible to add a way to undo it...

@DNin01
Copy link
Member

DNin01 commented Apr 9, 2024

Related addon idea: notification with undo button when deleting things or doing other destructive actions, including alphabetizing a sprite's costumes or sounds.

@DNin01 DNin01 mentioned this pull request May 4, 2024
@Jazza-231
Copy link
Contributor Author

Ctrl+Z can't usually undo costume deletions, additions, or reorders, AFAIK.

Well, I meant Edit > Restore Costume then. Of course, Scratch won't let you undo reorders, because you can only do one reorder at a time, so it's trivial to undo it manually. But if we're going to do multiple reorders with a click of a button, it sounds sensible to add a way to undo it...

I am about to add functionality to the edit menu to restore the previous order using restore

Copy link
Contributor

@Joeclinton1 Joeclinton1 left a comment

Choose a reason for hiding this comment

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

The while loop which keeps checking if it isSorted is really questionable. You should just sort it once, then directly sort it with reorder, there's no need to check if it's actually sorted if you could just sort it correctly in one pass. There absolutely should not be anything checking iteration counts.

I probably shouldn't have approved this, I missed the while loop logic.

@Jazza-231
Copy link
Contributor Author

Jazza-231 commented Jun 16, 2024

The while loop which keeps checking if it isSorted is really questionable. You should just sort it once, then directly sort it with reorder, there's no need to check if it's actually sorted if you could just sort it correctly in one pass. There absolutely should not be anything checking iteration counts.

I probably shouldn't have approved this, I missed the while loop logic.

Feel free to commit changes, ill give you contributor access. I seem to be having trouble with the whole idea of sorting and I'm not sure why

@Joeclinton1
Copy link
Contributor

Joeclinton1 commented Jun 16, 2024

The while loop which keeps checking if it isSorted is really questionable. You should just sort it once, then directly sort it with reorder, there's no need to check if it's actually sorted if you could just sort it correctly in one pass. There absolutely should not be anything checking iteration counts.
I probably shouldn't have approved this, I missed the while loop logic.

Feel free to commit changes, ill give you contributor access. I seem to be having trouble with the whole idea of sorting and I'm not sure why

If I'm making changes i'm going to redo most of your logic.

Before I do that, I'll just say how I would do it. I want the lists (this.sprite.sounds, this.sprite.costumes, this.runtime.targets) directly sorted with natural sort. No sorting of names or anything, just a direct sort and then a projectEmitUpdate.

@Jazza-231
Copy link
Contributor Author

If I'm making changes i'm going to redo most of your logic

Fair enough, if you want to do that then go ahead, feel free to add yourself to the credits too, I am genuinely fully stumped as to why its not working

+ reorganise code to be cleaner
+ handle modifying names and adding new costumes before reordering
+ make some functions more concise
+ remove unnecessary while loops / waitForElements
@Joeclinton1
Copy link
Contributor

Joeclinton1 commented Jun 18, 2024

I did the rewrite, seems to work now.

Perhaps an option could be added to settings for descending vs ascending sort?

Also the addon should be renamed to "sort assets by name" since "sort by alphabetical order" implies the existence of other sorts

@Jazza-231
Copy link
Contributor Author

Any ideas why the prettier check is failing?

@Joeclinton1
Copy link
Contributor

Any ideas why the prettier check is failing?

It's been failing on things recently

@Joeclinton1
Copy link
Contributor

Joeclinton1 commented Jun 18, 2024

I'm going to list the things that need to be tested for this addon:

  • Sort alphabetically appears if the assets are not sorted alphabetically for costumes, sounds,sprites. When clicked it sorts them correctly, and sorts the correct assets depending on which target pane you're on.
  • Sort alphabetically is grayed out if the assets in the target pane are already sorted
  • If sort alphabetically is applied, a button to restore the original order (restore order) replaces the normal undo button
  • when the restore order button is clicked, the order returns to what it was before the sort was applied
  • if restore order is clicked and new assets have been created between the original sort and the restore order, those assets remain in the same order but are placed at the end of the asset array
  • if restore order button is clicked, and assets have been renamed, those assets still return to their original order. Renaming other assets than the ones on the target pane for the target that was reordered has no effect.
  • if a different action is applied that has an undo function, this replaces the restore order button
  • the asset that was selected before applying sort alphabetically or restore order remains selected after it is applied

@Jazza-231
Copy link
Contributor Author

the asset that was selected before applying sort alphabetically or restore order remains selected after it is applied

Besides this which is about to be fixed, all tests pass.

@Joeclinton1
Copy link
Contributor

Joeclinton1 commented Jun 18, 2024

It still isn't reselecting on the reorder. I might have to use the react select from asset conflict dialog

@WorldLanguages
Copy link
Member

Also the addon should be renamed to "sort assets by name" since "sort by alphabetical order" implies the existence of other sorts

Have we considered adding the ability to sort numerically? That is, [1,3,10] instead of [1,10,3].

@Jazza-231
Copy link
Contributor Author

Jazza-231 commented Jun 22, 2024

Also the addon should be renamed to "sort assets by name" since "sort by alphabetical order" implies the existence of other sorts

Have we considered adding the ability to sort numerically? That is, [1,3,10] instead of [1,10,3].

This addon does that. We chose this method because sorting numerically is most useful in every single case, no matter what.
This is also why we do not use the .sort() method and implemented our own naturalSort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new addon Related to new addons to this extension. `scope: addons` should still be added. scope: addon Related to one or multiple addons status: needs review [Use when there's already 1 approval] Review needed on the PR type: enhancement New feature for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants