Palette import from GIMP .gpl, JASC .pal files! #329
Conversation
ghost
changed the title
ghost
changed the title
ghost
marked this pull request as ready for review
There was a problem hiding this comment.
Hi, had a quick look at it and made some tests. It works well and I would say code looks also good. Found some minor things you may have just overlooked to clean up and some missing error handling / user feedback, in case someone would load some invalid files (which probably never happen, but you never know)
|
|
||
|
|
||
| # ------------------------------------------------------------------------------------------------- | ||
| func import_palette() -> void: |
There was a problem hiding this comment.
Looks like this function is not needed, should be removed
| @@ -274,6 +276,8 @@ func _is_mouse_on_ui() -> bool: | |||
| on_ui = on_ui || Utils.is_mouse_in_control(_toolbar) | |||
| on_ui = on_ui || Utils.is_mouse_in_control(_statusbar) | |||
| on_ui = on_ui || Utils.is_mouse_on_window(_file_dialog) | |||
| on_ui = on_ui || Utils.is_mouse_on_window(_export_dialog) | |||
There was a problem hiding this comment.
Have you added this line intentional? As far as I can see, you have not touched the _export_dialog with your changes, therefore I'm not sure if it is needed to add it here.
| match filepath.get_extension(): | ||
| "gpl": | ||
| if !is_valid_gpl(palette_string): | ||
| return |
There was a problem hiding this comment.
I think it would be good to give the user some feedback in case a invalid file was loaded. At the moment when you load an invalid .gpl file the application freezes for a short time and then just resume without notify the user. That could be a bit confusing.
| palette_colors = parse_gpl_palette(palette_string) | ||
| "pal": | ||
| if !is_valid_pal(palette_string): | ||
| return |
There was a problem hiding this comment.
Same as for the invalid file load for .gpl
| return | ||
|
|
||
| palette = PaletteManager.create_custom_palette(palette_name) | ||
| if palette != null: |
There was a problem hiding this comment.
Either here or in the is_valid_gpl/is_valid_pal function a check if any color is in the palette should be done, because if you load a palette without any color inside you will run into an error later when the program tries to activate the new palette and set the active color to the first one.
There was a problem hiding this comment.
One last thing, if you go crazy and load a palette with 256 colors, you cannot access all colors, because the list gets too long. Probably there should be a scrollbar added. No need to do it with this pull request, but thought I should mention it.
ghost
closed this by deleting the head repository
In response to my own issue, #328
I like to use palettes from sites like lospec.com when doing anything graphical, and I thought it would be nice to add the feature here, it's the best open source whiteboard software I tried so far!
The implementation is probably a bit rough and I think I need some help to better organize the code.
I took the liberty of using a fitting icon from the Remix icon library that's already in use.
I could also make an export feature, but it's probably even more out of scope than this one already is.