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

Windows 3.1 style for configuration tool #5108

Merged
merged 42 commits into from
Jul 10, 2024

Conversation

aybe
Copy link
Collaborator

@aybe aybe commented Jul 9, 2024

Does this PR introduce new feature(s)?

Trying to apply pixel-perfect Windows 3.11 look'n'feel to CFGTOOL...

Buttons are up as well as their focus style.

Ideally,

  • extract/group/fix theme constants
  • sort messy help pages alphabetically
  • implement mouse wheel
  • simplify some aspects

Before:

image

After:

image

@aybe
Copy link
Collaborator Author

aybe commented Jul 9, 2024

By the way, not sure about that one nor when it started to happen:

f78010c

@joncampbell123
Copy link
Owner

Nice work! Here's an example of an actual Windows 3.1 application using the "3D" common controls DLL for color reference for future development.
krnl386_000

@maron2000
Copy link
Contributor

I wonder if we can make the buttons slightly thinner so that the Save and Close buttons don't get cut off.
The screenshot below is shown when window size is set to default.

image

@joncampbell123
Copy link
Owner

Either the configuration menu should enforce a minimum of 800x600, or perhaps all the buttons could be placed inside a scrollable region.

@maron2000
Copy link
Contributor

Seems to set gridbtnheight = 24 would be fine.

However on Linux, you can use the space used for the menu bar, so you have enough space with the default window size without changing the settings.
So maybe make it conditional for Windows.

image

@aybe
Copy link
Collaborator Author

aybe commented Jul 9, 2024

Nice work! Here's an example of an actual Windows 3.1 application using the "3D" common controls DLL for color reference for future development. krnl386_000

I've got a VM up and been using Paint.NET to grab every detail as I can!

@aybe
Copy link
Collaborator Author

aybe commented Jul 9, 2024

Either the configuration menu should enforce a minimum of 800x600, or perhaps all the buttons could be placed inside a scrollable region.

1st thing I guess would be to remove the header "Choose a settings...", it takes place and is useless.

Then to see where the checkbox could go, maybe it can work in menu or in other windows.

If so, then we gain 1 line immediately.

Also maybe, window can become 720 px wide with maybe slightly smaller labels, e.g. IDE #1, LPT, COM.

If all these work we might be able to have 5 columns and would gain lot of space for eventual future sections.

@aybe
Copy link
Collaborator Author

aybe commented Jul 9, 2024

I wonder if we can make the buttons slightly thinner so that the Save and Close buttons don't get cut off. The screenshot below is shown when window size is set to default.

image

Seems to set gridbtnheight = 24 would be fine.

However on Linux, you can use the space used for the menu bar, so you have enough space with the default window size without changing the settings. So maybe make it conditional for Windows.

image

Problem is, if below 26 then focus while pressed is cropped... And if you adjust label positions it looks really squished.

Then you have margins between buttons that must be kept else it looks horrible.

Noticed that, not sure why on default config with TTF save/close are not cropped:

image

@maron2000
Copy link
Contributor

maron2000 commented Jul 9, 2024

Hmm, so it's kind of complicated.
One idea is to remove the 'Choose a setting...' line, and move the checkbox next to the 'Save' & 'Close' buttons.
Or simply resize the window while displaying the configuration tool.

@aybe
Copy link
Collaborator Author

aybe commented Jul 9, 2024

Before:

image

After:

image

Before:

image

After:

image

Summary:

  • made a Win31 pixel perfect button, colors, focus
  • overhauled main window so it fits by default, it's exactly its size now
  • added Theme class and upgraded all Color:: to it
  • adjusted all controls so they are aligned on the right

I think that's enough for the PR...

Reference for future improvements:

  • remove useless "Configuration for", "Click help button" at top/bottom of each window
  • implement mouse wheel
  • sort properties by name
  • thoroughly apply win31 theme
    • maybe keep older gray since it's less annoying
  • rename/nice sort main window sections
  • IMFC is empty, could be removed so it's even neater
  • dark theme?
  • ...

@aybe
Copy link
Collaborator Author

aybe commented Jul 10, 2024

Final version:

image

image

image

Fixed layout, sorted help/properties, revamped main window/labels.

Mouse wheel does not work, it needs window to be clicked at least once for it to work, not sure why:

e5058a8

Dark theme:

Not for now, not easy at all to come up with the right colors...

Full Windows 3.1 theme:

Not for now, I did however extract all themes from CONTROL.INI, will need quite some refactoring.

d628570


Beside mouse wheel issue, things look good, I guess it's enough for this PR.

@aybe aybe marked this pull request as ready for review July 10, 2024 02:15
Copy link
Contributor

@maron2000 maron2000 left a comment

Choose a reason for hiding this comment

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

The test builds are built successfully with the fixes.

@@ -2272,25 +2333,27 @@ bool ScreenSDL::event(SDL_Event &event) {
}
lastdown = 0;
return rc;
}
case SDL_MOUSEWHEEL:
Copy link
Contributor

Choose a reason for hiding this comment

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

SDL_MOUSEWHEEL not available on SDL1?
Maybe add #if defined(C_SDL2)

gui_tk.cpp:2336:10: error: use of undeclared identifier 'SDL_MOUSEWHEEL'
    case SDL_MOUSEWHEEL:
         ^
gui_tk.cpp:2337:33: error: no member named 'wheel' in 'SDL_Event'
        return mouseWheel(event.wheel.y);

bool WindowInWindow::mouseWheel(int wheel)
{
// BUG requires to click at least once in window for it to work
scroll_pos_y = min(max(scroll_pos_y - wheel * 15, 0), scroll_pos_h);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add #include <algorithm> header, and use std::min, std::max instead?

gui_tk.cpp:2880:20: error: use of undeclared identifier 'min'; did you mean 'fmin'?
    scroll_pos_y = min(max(scroll_pos_y - wheel * 15, 0), scroll_pos_h);
                   ^~~
                   fmin

@joncampbell123
Copy link
Owner

Ready?

@maron2000
Copy link
Contributor

Thank you for the fix. I'm happy that the cropped window is fixed as well as the improved look & feel.

@joncampbell123 joncampbell123 merged commit 5211ebb into joncampbell123:master Jul 10, 2024
20 checks passed
@aybe
Copy link
Collaborator Author

aybe commented Jul 10, 2024

Nice!

Just two things I'm not happy with:

This is really a dirty fix I did by just swapping the operands, it happens during ENC obviously so I am wondering whether it's simply due to it or not, searched online but didn't find much about it:

f78010c

Seems like the GUI would need good refactoring to implement mouse wheel without clicking it first, correct?

(btw, just noticed that one can flick scroll, obviously, it's been clicked at this point so that doesn't help much)

e5058a8

@aybe aybe deleted the cfgtoolwin31style branch July 13, 2024 15:26
@Torinde Torinde mentioned this pull request Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants