search

jkotlinski / lsdpatch

LSDj Patcher
http://www.littlesounddj.com
Other
173 stars 14 forks source link

Multiple Improvments #28

Closed Eiyeron closed 7 years ago

Eiyeron commented 7 years ago

(Note for future me : never push PR with a master branch ever)

Edit: here's some more improvments I added.

<end of generated PR message, the human starts writing here> Hello.

So, designing a whole font felt a bit weird so I went on and added some features to the font edition, such as importing/exporting bitmap and supporting right click on the tile editor to color with another color.

Oh and you might need java 8 to run it as i'll use lambdas. According to this site, more than 70% JVMs used are >= 8.0, so it might be good for both our sanity and the code readability.

I'll probably add to my own backlog a refactor of parts of the code of Patcher, but it won't probably be for now.

Have a nice day!

jkotlinski commented 7 years ago

Hi, I'm sorry but I cannot accept Java 8 contributions. Some users are stuck with old JRE. (E.g. using old OS or working on university computers)

I would love to see a new contribution that does not increase the JRE requirement!

All the best Johan

On Thu, 27 Jul 2017 at 14:57, Florian Dormont notifications@github.com wrote:New radio buttom ramp for right click

  • Right click on the tile editor will use the second color
  • Four buttons have been added to shift a tile's data
  • Supports now bitmap import/export (in File menu) <end of generated PR message, the human starts writing here> Hello.

So, designing a whole font felt a bit weird so I went on and added some features to the font edition, such as importing/exporting bitmap and supporting right click on the tile editor to color with another color.

Oh and you might need java 8 to run it as i'll use lambdas. According to this https://plumbr.eu/blog/java/java-version-and-vendor-data-analyzed-2017-edition site, more than 70% JVMs used are >= 8.0, so it might be good for both our sanity and the code readability.

I'll probably add to my own backlog a refactor of parts of the code of Patcher, but it won't probably be for now.

Have a nice day!

You can view, comment on, or merge this pull request online at:

https://github.com/jkotlinski/lsdpatch/pull/28 Commit Summary

  • Font Editor : right click, shift and bitmap im/export

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jkotlinski/lsdpatch/pull/28, or mute the thread https://github.com/notifications/unsubscribe-auth/ADG-O9y4wxGeULUd4cYl8lMHF80uk_T5ks5sSIkvgaJpZM4OlPmN .

--

http://www.littlesounddj.com

Eiyeron commented 7 years ago

Hello.

Thanks for the fast answer. Which version do you target then, 6, 7 or older?

Best regards.

jkotlinski commented 7 years ago

JRE 1.6 - I guess that's Java 6?

On Thu, 27 Jul 2017 at 15:47, Florian Dormont notifications@github.com wrote:

Hello.

Thanks for the fast answer. Which version do you target then, 6, 7 or older?

Best regards.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/jkotlinski/lsdpatch/pull/28#issuecomment-318367124, or mute the thread https://github.com/notifications/unsubscribe-auth/ADG-O7k8M7sWx4l8sMLEQg5OoObg2VZNks5sSJUEgaJpZM4OlPmN .

--

http://www.littlesounddj.com

Eiyeron commented 7 years ago

Gotcha, I'll lower the compilation version. Thanks for the feedback!

Eiyeron commented 7 years ago

It should be good now. Can you confirm if it works on older Java versions?

jkotlinski commented 7 years ago

The features are good but the UI could use a little polish. How about making the color selection work like GBTD? http://www.devrs.com/gb/hmgd/gbtd.html There, you e.g. click left mouse button on white color field to map it to white. Basically just having three color fields that are clickable with left and right button would be enough.

Also, shift buttons would be nice to have as icons if possible, because currently they take a lot of screen space and look kind of ugly. What do you think?

Eiyeron commented 7 years ago

I'm taking notes of such suggestions and adding them to my own roadmap. I have to admit I didn't really know what to do for the shift buttons.

Eiyeron commented 7 years ago

As of the latest commit, both your suggestions have been implemented. image

jkotlinski commented 7 years ago

Hi! I tried to compile with 1.6 but I get various compile errors:

.\fontEditor\FontEditor.java:39: error: type JComboBox does not take parameters
        private JComboBox<String> fontSelector;
                         ^
.\fontEditor\FontEditor.java:142: error: type JComboBox does not take parameters
                fontSelector = new JComboBox<String>();
                                            ^
.\fontEditor\FontEditor.java:400: error: type JComboBox does not take parameters
                } else if (cmd == "comboBoxChanged" && e.getSource() instanceof JComboBox<?>) {
                                                                                         ^
.\fontEditor\FontEditor.java:402: error: type JComboBox does not take parameters
                        JComboBox<String> cb = (JComboBox<String>) e.getSource();
                                 ^
.\fontEditor\FontEditor.java:402: error: type JComboBox does not take parameters
                        JComboBox<String> cb = (JComboBox<String>) e.getSource();
                                                         ^
.\fontEditor\FontEditor.java:406: error: type JComboBox does not take parameters
                } else if (cmd == "comboBoxEdited" && e.getSource() instanceof JComboBox<?>) {
                                                                                        ^
.\fontEditor\FontEditor.java:408: error: type JComboBox does not take parameters
                        JComboBox<String> cb = (JComboBox<String>) e.getSource();
                                 ^
.\fontEditor\FontEditor.java:408: error: type JComboBox does not take parameters
                        JComboBox<String> cb = (JComboBox<String>) e.getSource();
                                                         ^
.\fontEditor\FontMap.java:52: error: cannot find symbol
        tileZoom = Integer.min(getWidth()/LSDJFont.FONT_MAP_WIDTH , getHeight()/LSDJFont.FONT_MAP_HEIGHT);
                          ^
  symbol:   method min(int,int)
  location: class Integer
.\fontEditor\FontMap.java:53: error: cannot find symbol
        tileZoom = Integer.max(tileZoom, 1);
                          ^
  symbol:   method max(int,int)
  location: class Integer
.\fontEditor\TileEditor.java:121: error: cannot find symbol
        int minimumDimension = Integer.min(getWidth(), getHeight());
                                      ^
  symbol:   method min(int,int)
  location: class Integer
.\fontEditor\TileEditor.java:138: error: cannot find symbol
        int minimumDimension = Integer.min(getWidth(), getHeight());
                                      ^
  symbol:   method min(int,int)
  location: class Integer
.\fontEditor\TileEditor.java:155: error: cannot find symbol
        int minimumDimension = Integer.min(getWidth(), getHeight());
                                      ^
  symbol:   method min(int,int)
  location: class Integer
Note: FileDrop.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
13 errors
Eiyeron commented 7 years ago

Thanks for the feedback, it seems that even though I'm using 1.6 compliance settings, it doesn't totally works. I'm working on fixing this.

Edit : Should be okay now. I'm testing with a J6 JRE and got the same errors.

jkotlinski commented 7 years ago 
$ java -jar LSDPatcher.jar
Exception in thread "main" java.lang.NoClassDefFoundError: fontEditor/FontEditor
        at MainWindow.<init>(MainWindow.java:42)
        at LSDPatcher.<init>(LSDPatcher.java:37)
        at LSDPatcher.main(LSDPatcher.java:68)
Caused by: java.lang.ClassNotFoundException: fontEditor.FontEditor
        at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
        ... 3 more
Eiyeron commented 7 years ago

Are you compiling yourself your Jar? I may have missed a few classes in the provided shell script. I tried to update it to include also the subfolders' classes.

A better solution would be using a Makefile-like solution to automatically discover the source files in the source tree and bundle the compiled classes together into a jar but as the tool doesn't have a big scope, it might be fine to manually update the script.

Eiyeron commented 7 years ago

Updated a bit the PR as its title doesn't fit the content at all anymore.

jkotlinski commented 7 years ago

@Eiyeron I compile with "sh make-jar.sh" (simply executing the commands in that file) then run with "java -jar LSDPatcher.jar" I still get the same error when running.

Eiyeron commented 7 years ago

Ok, I think I got it : putting classes into packages supposes that the .class files must be into folders in the .jar (like FontEditor.class must be in patcher.jar/fontEditor/). Your script doesn't allow that, sadly.

I can't have access to javac on windows for now, I'll try to fix that later. But thanks for the feedback!

jkotlinski commented 7 years ago

I am closing this pull request without merging now, since the project does not compile & run successfully on my machine. Contributions are still welcome but would prefer to have them submitted as individual features. Thanks for your attempts to evolve the project further!

Eiyeron commented 7 years ago

I agree with closign the PR, I ended up mistakenly doing more work on the branch without checking if it wouldn't be shown on the PR.

But as now, I wouldn't be able to separate the features I worked on as they are also based on the refactor.

jkotlinski commented 4 years ago

Hi! Just wanted to say - a couple of years passed by now, and hopefully not many use Windows XP anymore. So maybe a switch to Java 8 is fine. I don't think I would oppose that anymore :-)

Eiyeron commented 4 years ago

Hello. Glad to see you're not giving up on that.

Honestly, on my side, I am not yet content with the looks on the code itself, it's halfway factorized into a way of structuring I had two years ago I don't have anymore. I'd not mind at all merging the forks together as it'd make less confusion on which tool to use, I'm just not feeling my code quality is par to what I expect to offer for a PR anymore.

I'm also halfway in UI redesign process on my side, half as an experiment, half as an attempt at making a "better" (citation needed) UI for our chiptuners' customization folly. You can check that on the refresh PR

The other things I don't know if it's going to hinder the merge back is that I'm using a UI library to handle a bit better Swing's internals and I tried to use a Maven-powered build system to simplify the jar packing process.

For now, I'll try in making the backlog way smaller before opening a new PR, is that okay with you?

Have a nice day!

jkotlinski commented 4 years ago

Hi! For sure. There is absolutely 0% pressure from my side :-) I simply wanted to share my updated standpoint on Java 6/8. One note about the new UI mockup. I know ”edition” is commonly used to describe ”editing” in France. In the rest of the English speaking world, though, ”edition” means ”version”. So some alternate wording might be in place unless Americans get confused. All the best J

On Sat, 14 Dec 2019 at 21:44, Florian Dormont notifications@github.com wrote:

Hello. Glad to see you're not giving up on that.

Honestly, on my side, I am not yet content with the looks on the code itself, it's halfway factorized into a way of structuring I had two years ago I don't have anymore. I'd not mind at all merging the forks together as it'd make less confusion on which tool to use, I'm just not feeling my code quality is par to what I expect to offer for a PR anymore.

I'm also halfway in UI redesign process on my side, half as an experiment, half as an attempt at making a "better" (citation needed) UI for our chiptuners' customization folly. You can check that on the refresh PR https://github.com/Eiyeron/lsdpatch/pull/41

The other things I don't know if it's going to hinder the merge back is that I'm using a UI library to handle a bit better Swing's internals and I tried to use a Maven-powered build system to simplify the jar packing process.

For now, I'll try in making the backlog way smaller before opening a new PR, is that okay with you?

Have a nice day!

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/jkotlinski/lsdpatch/pull/28?email_source=notifications&email_token=AAY34O7FMTCZIN6Y2J2XTOLQYVAUBA5CNFSM4DUU7GG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4K6SQ#issuecomment-565751626, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAY34O3EELEWBA4CHTGMK3DQYVAUBANCNFSM4DUU7GGQ .