-
Notifications
You must be signed in to change notification settings - Fork 6
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
UI Improvements #2
base: master
Are you sure you want to change the base?
Conversation
Changed Blue Channel button color to better reflect bevel refactored code for easier adaptability
@@ -48,6 +53,24 @@ of this software and associated documentation files (the "Software"), to deal | |||
* @param args the command line arguments | |||
*/ | |||
public static void main(String[] args) { | |||
// Check for savepath persistence file | |||
File f = new File( "savepath.txt" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, but could you put it in a separate function? For the sake of keeping the main function tidy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There you go :)
} | ||
|
||
public static void ShowError( String msg, String headline ) { | ||
JOptionPane.showMessageDialog(null, msg, headline, JOptionPane.ERROR_MESSAGE ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to move these two functions to the Gui class? Or at least the lines that draw the JOptionPane? I'm not fond of the swing element class being used directly by the backend class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getFilenameFromPath is definitely a backend method - only its result is used in the frontend. And calling "TextureRemix" a "backend" class is a little strange, given all the manipulations on the UI this class does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I was referring to ShowError
and ShowMessage
, not the one above it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, there you go.
Now I've had a chance to test it out properly, so I can give some proper feedback. As I said before, I like the persistent file path feature. It worked without a hitch. The new popups for showing errors and messages are good. Maybe having a pop-up like this isn't ideal, but I'd rather have this than no feedback whatsoever. Showing the image name isn't a bad idea, but the code needs to be improved a bit. If you pick a file with a long name, it stretches the box to fit the name, which the window cannot accommodate if all four input images are loaded. Could you have it truncate the displayed file name so that the input box is not stretched? Because of the variable-width font, a solution that masks the text after a fixed width might be a good approach if possible. I don't particularly like the changes to the color buttons, however. I do like the visual feedback of showing which button is active, but I don't like raised bevels. The indented beveling is an important part of the visual design. I also don't like colors being changed, as each button/port represents pure red/green/blue. However, I think it would be a good idea to combine these two changes: rather than the buttons being raised and lowering when active, the buttons could "light up" when active. That way, the user is given important visual feedback for their actions without changing the central design. What do you think? |
@PopUps: If you have a better idea, go ahead. I have made the "Job done"-Popups just because I was missing feedback on a completed operation, which is rather pointless on smaller files, but I work with PC Textures 2048² and up, which take some time to process. Remove them if you want to. @Imagename: I'm sure there is an option to keep the width fixed for a Textbox, I'll look into it when I find the time, but I'm sure you can find it yourself as well. @Changed Color: I did explain why I changed the color, and changing the Bevel was the easiest feedback I could come up with. I'm well aware what these buttons represent, and I strongly believe that the users of your software are wellinformed enough to be able to recognize the button as "blue channel" despite its color not being 0000FF. But it is your software, I am not invested in the change, so if you don't like it, just change it to the way you want it. I have tried to "centralize" the process of highlighting and resetting the button, so the change is really combined in SetSelection and ResetSelection. In general: I made these changes to help you out and because I liked the approach of a simple software capable of splitting and recombining channels. It is usable for me as it is now, with the changes I made to my version. You are welcome to use these changes, they are open source and free to use, and if you don't even mention my involvement, I'm perfectly fine with it. But I believe you are measuring my expansions to your software with a significantly higher standard than what you came up with on your own, e.g. using Textboxes as Buttons and explaining some of their uses in a forum thread you need to find on google. So please forgive me that my motivation of meeting your requests is experiencing a steep decline. |
Pop-ups: These are good for now, as I have no better alternative right now. Perhaps I should have phrased my response better. Color: I didn't see an explanation for the color change before. After reading your response, I dug deeper and found it in the commit's comment section. I understand the reasoning. General: I apologize for how I have handled responding to this so far. I am used to a different work flow for projects, which is why I asked you to make the changes rather than fixing them myself. I do greatly appreciate your contribution, and I want to see your improvements merged, though I do not want to approve changes that compromise the design. I admit fault for not making that design plan available anywhere, and I don't blame you for not knowing. As far as standards go, the recent additions definitely go against my own standards and design plans. (That hollow box is supposed to be where a preview of the image goes.) I felt it more important to get the new features available as soon as possible. I understand if you do not wish to make any more changes. I'll see what I can do when I have the time. |
Seemed to be the last remaining point of concern. Hope it helps. |
Did you have time to review the latest changes? |
I have not yet, unfortunately. I have been rather busy lately. |
Dear Peardian,
I found your little tool today and tried to use it for my purposes. It didn't work for some reason, so I dug around a little and found several things uncommunicated to the user. I therefore made some changes, I hope you like them and integrate them. I tried to be verbose in naming, I did not comment my changes, but I believe most of them are rather obvious:
I'm not much of a java coder either, my domain is C#, but I believe the changes I've made are sound and in line with your guidelines. I'd be happy if you would accept my pull request!
kind regards
Chris
P.S.: I also created a branch for myself called "integrateTGA" where I used tmyroadctfig/com.realityinteractive.imageio.tga to also enable your little program to load TGA files and handle them correctly, but I was unsure as to your policy on this, which is why I made the branch just for me. If you would find that useful too, just let me know!