sylikc / jpegview

Fork of JPEGView by David Kleiner - fast and highly configurable viewer/editor for JPEG, BMP, PNG, WEBP, TGA, GIF and TIFF images with a minimal GUI. Basic on-the-fly image processing is provided - allowing adjusting typical parameters as sharpness, color balance, rotation, perspective, contrast and local under-/overexposure.
Other
2.16k stars 127 forks source link

Feature request: UI feedback on alwaysontop toggle, save as jxl #119

Open LaurentGrenet opened 1 year ago

LaurentGrenet commented 1 year ago

Hi @sylikc

first of all, thank you for this new version. It's a very good thing that someone (you...) makes this super soft living and evolving. Moreover, you imagined that this new version were a gift for lunar new year : it's even better than that : it is out the day of my birthday ! So thanks a lot for your birthday gift !

Some (little...) remarks now :

1) I've been a little bit disappointed that fixes I sent you 3 weeks ago for french translation were not in this new version. No issue for me, since I kept a copy, but please insert them ASAP in official branch to do not lose them...

2) With this new version, come two new entries in .ini file. But since they have not been reported in the template file (JPEGView.ini.tpl), they are not copied in the user .ini file when using Settings/Admin / Update user settings... We are then obliged to copy them manually in the user .ini file if we want to be able to change their value. FYI, I did a test : if they were defined in the .tpl file, it would have been enough to have them propagated correctly to the user .ini file with the admin menu. So it is worth keeping this .tpl file up-to-date (or maybe, even better, to remove it but have the "Update user settings..." relying on the global JPEGView.ini, rather than the .tpl one : BTW I do not understand the need to have both files, except to be the cause of the bug I mention... But maybe there is another valuable reason...?)

3) Among new features, at least one (Hide window title bar) would worth to be present in the pop up menu (right clic) and to have an entry in .ini file, to be able to define it as a default value (while today, we cannot have title bar hidden at tool launch)

4) And about the new feature "Always on top", it's a little bit annoying that nothing in the title bar shows that we are in this mode. (For this one, I don't think that an entry in .ini file makes sense, since I cannot imagine define "always on top" as a default, but it would be probably worth as well to have an entry in the pop up menu)

5) BTW, these two modes are not shown in the JPEGView Help (F1) contrary to a lot of other options (like F2, F3, etc...) for which current situation (on or off) is displayed in the help pop up 2023-01-27 16_54_00-JPEGView Help

sylikc commented 1 year ago

So thanks a lot for your birthday gift !

Happy birthday! Thanks for keeping up to date and giving me all this feedback... it's the enthusiastic userbase that keeps me motivated!

  1. I've been a little bit disappointed that fixes I sent you 3 weeks ago for french translation were not in this new version. No issue for me, since I kept a copy, but please insert them ASAP in official branch to do not lose them...

Oh. I forgot to include the new FR translation files, mb. I'll get them put in. The next release is upcoming waiting on a few more changes on #109. I was hoping to get qbnu's HEIC implementation in this release, but there's some things open there that needs discussion

  1. With this new version, come two new entries in .ini file. But since they have not been reported in the template file (JPEGView.ini.tpl), they are not copied in the user .ini file when using Settings/Admin / Update user settings... We are then obliged to copy them manually in the user .ini file if we want to be able to change their value. FYI, I did a test : if they were defined in the .tpl file, it would have been enough to have them propagated correctly to the user .ini file with the admin menu. So it is worth keeping this .tpl file up-to-date (or maybe, even better, to remove it but have the "Update user settings..." relying on the global JPEGView.ini, rather than the .tpl one : BTW I do not understand the need to have both files, except to be the cause of the bug I mention... But maybe there is another valuable reason...?)

So... I didn't realize that. I, myself, don't use that Settings/User INI stuff so I didn't realize that not keeping the rest of the files up-to-date causes weird glitches. That whole INI madness is actually kinda convoluted in code. I think I have a better understanding this year than last year, but it's still a headache that I haven't had the courage to try to fix.

See #40 for an explanation of what's going on. Basically, there's a few ways that INIs can be loaded in order... from the TPL -> user. Or from the system -> user, or like from a system -> user -> command line... etc etc... long story short, there's lots of ways and none of them are consistent. In my use case, where I'm keeping JPEGView.ini up to date, it ignores the TPL file entirely. The INI/Keymap logic is on my list of todo's to entirely redo, but I haven't had to time to attack it yet, since the original author built so many ways of functionality into it that it's really a mess in code. And this doesn't even include the KeyMap.txt... which doesn't have this type of "cascading" functionality!

  1. Among new features, at least one (Hide window title bar) would worth to be present in the pop up menu (right clic) and to have an entry in .ini file, to be able to define it as a default value (while today, we cannot have title bar hidden at tool launch)

The option is in the right click menu. I put it into the Zoom area, because I couldn't figure out where to put it. The main menu is already too long as it is. I know someone would ask for it to be a default value eventually, but I figured have it as a feature people either love or hate (there's some glitches with it too) and then put in defaults later on enough interest.

  1. And about the new feature "Always on top", it's a little bit annoying that nothing in the title bar shows that we are in this mode. (For this one, I don't think that an entry in .ini file makes sense, since I cannot imagine define "always on top" as a default, but it would be probably worth as well to have an entry in the pop up menu)

This one's going to take some work. This is related to #116 actually, and in that discussion, the user is asking for feedback on option changes. The most logical place to put this information is the title bar at this point, but a sort of "cool fading popup text" drawn on the window would be nice, but pretty difficult to code. The title bar is great, but now I added a feature where you can hide the title bar! ... so... sigh.

  1. BTW, these two modes are not shown in the JPEGView Help (F1) contrary to a lot of other options (like F2, F3, etc...) for which current situation (on or off) is displayed in the help pop up

This is a bug, I will have to take a look.

Thanks for your support! All the feedback is great, even if I can't get to it just yet.

LaurentGrenet commented 1 year ago

Hi,

thank you for answering !

My comments

1) About .ini file management, I am not completely OK with @Owyn in his comment of #42 (https://github.com/sylikc/jpegview/issues/40#issuecomment-1037117730), because JPEGView is not always "installed" with installer, but sometimes only copied somewhere (it's my case) as a portable application. Nevertheless, his principles are good. How to manage that : we could imagine that each time JPEGView is launched, a check is done : Is the .ini file exist ? If No, create it with default values, but if yes, check if it is relative to the current version (a new entry in the file would contain the app version in which file has been last updated). If yes again, nothing to do, else update it with missing entries (if any) and update its "current version" reference I think this wouldn't take more time than now, even if done at each startup, since in any case, at each launch JPEGView has to read the .ini file, and the "supplementary workload" (update the file) would occur only in case the file is relative to an old version. And you could even apply this new process to the personal .ini file, if it exist.

The global process would be then, at each launch

With such a process, no need to keep the menu entry to force personal .ini file update with new values. Only the one to create a personal ini file, in case it doen't exist before.

And, of course, apply all this process not only to .ini file, but also to keymap file.... or merge keymaps definition in the ini file (and then have only one file to manage...)

2) About the new features : You're right, they are in the menu (and behind zoom is a good place), but I didn't see them, sorry of that.

3) About the capability to define "without title" as a default, to be clear, I surely don't ask it becomes the global default value for the application, but only give a mean to users to say : this is now (until I change it) the default value for my laptop (global .ini file) or for me (personal .ini file)

4) About "visibility" of "always on top", I remove my request. Actually, I have seen that when a window is set "on top" with some external tool, it is never "reminded" in the title bar. And since actually it's always the result of a voluntary action, there is no real need to "remind" it (except maybe for users with Alzheimer's disease...). On the other hand, about #116 , I'm strongly against the idea to add too much info in the title bar : it has to remain "soft".

sylikc commented 1 year ago

For the INI management, I forgot to add another complication... localization.

There's actually code in there to load the JPEGView_XX.ini for localization. If it doesn't exist, it falls back to JPEGView.ini if it exists...

It's actually pretty convoluted in code. Users can use a command line parameter to override the default behavior, etc etc. I'll take all the inputs into consideration... it's just not ok the way it is now, any improvement would be better 😊

mdnava commented 1 year ago

@sylikc Hello.. I just tried the new version with JPEG XL support, great! great! I had been using WIC, but adding JXL support makes the program again live up to its name. Please consider sometime in the future adding an option to Save As .JXL (I think it would require new options in JPEGView.ini).

If I may add something to the subject at hand about the config file..

I believe JPEGView has never updated the .ini file on its own when in portable mode. I'm not sure when it uses APPDATA or other location, but I think it never does, and I believe it would be best to keep that standard. I also think JPEGView.ini file is only created from the template when one is not found within the preset locations. But whatever decision, if the .ini file is not modified with new options,. At the very least least the "JPEGView.ini.tpl" file should always have all the available options (including newer ones) with their default settings. It's likely how it was meant to work.

In my case I just took note of the changelog and added the new settings and shortcuts. However, the "ShowFullPathInTitle" did not seem to work for me, and the IDM_CHANGESIZE keymap is already available in v1.0.37 so I don't understand the addition.

sylikc commented 1 year ago

I just tried the new version with JPEG XL support, great! great! I had been using WIC, but adding JXL support makes the program again live up to its name.

Thanks @mdnava JPEG XL support brought to you by @qbnu ... super awesome work.

In my case I just took note of the changelog and added the new settings and shortcuts. However, the "ShowFullPathInTitle" did not seem to work for me, and the IDM_CHANGESIZE keymap is already available in v1.0.37 so I don't understand the addition.

Whoops number 2... I didn't check my work. Sometimes when people ask for a feature I forget to check if it's already there... Thanks for the catch lol.

LaurentGrenet commented 1 year ago

I believe JPEGView has never updated the .ini file on its own when in portable mode

Maybe not really "on its own", but with the menu "Settings/Admin / Update user settings..." the personal .ini file (stored in %appdata%) is updated with new entries coming from .tpl file.

And I take the opportunity of this post to confirm that, contrary to @mdnava , "ShowFullPathInTitle" works fine.

mdnava commented 1 year ago

"ShowFullPathInTitle" works fine

@LaurentGrenet – Like: ShowFullPathInTitle=true ?

LaurentGrenet commented 1 year ago

Actually, the name of the option is not "ShowFullPathInTitle" but "ShowFilePathInTitle" and, yes, it works fine.

In his comment when announcing 1.1.42, @sylikc wrote "ShowFullPathInTitle", but in the delivered JPEGView.ini file, the name is ShowFilePathInTitle

mdnava commented 1 year ago

Actually, the name of the option is not "ShowFullPathInTitle" but "ShowFilePathInTitle" and, yes, it works fine.

In his comment when announcing 1.1.42, @sylikc wrote "ShowFullPathInTitle", but in the delivered JPEGView.ini file, the name is ShowFilePathInTitle

Ok. With "ShowFilePathInTitle" it now works.. 👍

sylikc commented 1 year ago

Actually, the name of the option is not "ShowFullPathInTitle" but "ShowFilePathInTitle" and, yes, it works fine.

doh. I really goofed the release lol. Let me fix those including the CHANGELOG. I did a rename of the option halfway through adding the feature

sylikc commented 1 year ago

Hide title bar mode implemented in https://github.com/sylikc/jpegview/pull/243 and available in 1.3.46... other feature requests remain in backlog at this time