-
Notifications
You must be signed in to change notification settings - Fork 22
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
Update FLTK to 1.4.1 #87
base: master
Are you sure you want to change the base?
Conversation
Makefile
Outdated
fltk-config = $(bindir)/fltk-config | ||
|
||
CXXFLAGS = -std=c++17 -I$(srcdir) -I$(resdir) $(shell $(fltk-config) --use-images --cxxflags) | ||
LDFLAGS = $(shell $(fltk-config) --use-images --ldflags) $(shell pkg-config --libs libpng xpm) |
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.
Using $(bindir)/fltk-config
avoids the need for export PATH="$PWD/bin:$PATH"
in INSTALL.md and .github/workflows/main.yml
INSTALL.md
Outdated
# Build FLTK 1.4.0 | ||
git clone --branch release-1.4.0rc1 --depth 1 https://github.com/fltk/fltk.git lib/fltk | ||
pushd lib/fltk | ||
cmake -D CMAKE_INSTALL_PREFIX="$(realpath "$PWD/../..")" -D CMAKE_BUILD_TYPE=Release -D FLTK_GRAPHICS_CAIRO=1 -D FLTK_BACKEND_WAYLAND=0 |
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.
Updated this to clone into lib/fltk
instead of fltk
(to match the Windows instructions).
(also updated in .github/workflows/main.yml so that the cmake
command is identical) (Edit: the cmake command is no longer identical because the github workflow also skips building fltk_gl and fltk tests.)
@@ -69,7 +70,9 @@ int main(int argc, char **argv) { | |||
#ifdef _WIN32 | |||
SetCurrentProcessExplicitAppUserModelID(MAKE_WSTR(PROGRAM_AUTHOR) L"." MAKE_WSTR(PROGRAM_NAME)); | |||
#endif | |||
Fl::keyboard_screen_scaling(0); |
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.
Fl::keyboard_screen_scaling(0);
disables the new FLTK window scaling keyboard shortcuts Ctrl -/+ since Ctrl + is already in use by the magnifying glass icon.
FLTK's high DPI rendering support is still in effect if the display is already scaled by the OS instead of by the user.
Fl::visual(FL_DOUBLE | FL_RGB); | ||
fl_contrast_level(50); |
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.
FLTK has a newer improved contrast algorithm, and a new default contrast level of 39 instead of 50 (which is a good thing). But we set it back to 50 anyway because it works best for all 12 themes, particularly in the menubar.
@@ -2370,7 +2371,7 @@ static void use_rose_gold_colors() { | |||
Fl::background2(0xFF, 0xFF, 0xFF); | |||
Fl::foreground(0x4C, 0x1E, 0x12); | |||
Fl::set_color(FL_INACTIVE_COLOR, 0x60, 0x4C, 0x4C); | |||
Fl::set_color(FL_SELECTION_COLOR, 0x15, 0x81, 0xFA); | |||
Fl::set_color(FL_SELECTION_COLOR, 0x14, 0x79, 0xEA); |
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.
These 4 slight color tweaks allow the contrast level of 50 to actually work (as opposed to needing to raise the contrast level even higher, which would have other negative effects).
src/themes.cpp
Outdated
fl_rect(x+1, y+1, w-2, h-2); | ||
fl_line_style(FL_SOLID, 2); | ||
fl_rect(x+1, y+1, w-1, h-1); | ||
fl_line_style(FL_SOLID); |
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.
These 4 changes to drawing rects in the high contrast theme are necessary for the high contrast theme to look correct when high DPI screen scaling is in effect. Otherwise, there could be unexpected gaps between the two consecutive rectangles. This is fixed by drawing a single rectangle with a width of 2 instead. (Edit: this was re-done in an even better way; draw 4 rectangles -- one for each edge -- each with a width or height of 2.)
@@ -3186,7 +3191,7 @@ void OS::use_native_fonts() { | |||
use_any_font(FL_COURIER, monospace_fonts, _countof(monospace_fonts)); | |||
// Use common bold monospace font | |||
const char *bold_monospace_fonts[] = { | |||
"Ubuntu Mono bold", "Noto Sans Mono bold", "Droid Sans Mono bold", "DejaVu Sans Mono bold" | |||
"Ubuntu Mono Bold", "Noto Sans Mono Bold", "Droid Sans Mono Bold", "DejaVu Sans Mono Bold" |
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.
This change became necessary after enabling Cairo+Pango drawing on X11 (instead of plain X11 drawing).
This comes from -D FLTK_GRAPHICS_CAIRO=1
in the cmake
command.
Enabling Cairo+Pango on X11 isn't strictly necessary, but it avoids some bugs/quirks with plain X11 drawing and it looks better overall.
} | ||
fl_pop_clip(); | ||
} |
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.
The "origin" of this change is simply that the Fl_Label
struct had a few new fields added to it (h_margin_
, v_margin_
, and spacing
) which were all uninitialized in this code.
This resulted in the "Palettes" dropdown being drawn incorrectly/unpredictably.
Instead of "just" fixing the uninitialized data, I rewrote this based on the latest Fl_Choice::draw()
while preserving most of the customizations, since there are substantial useful changes in 1.4.0 especially in the first half.
I couldn't fully decipher the purpose the custom changes to the second half (the displacement of fl_push_clip()
/fl_pop_clip()
, the recalculation of ww
on old line 380) so I just went with the latest Fl_Choice::draw()
and the final result looks the same to me AFAICT. If you have a reason for these customizations, let me know.
I originally did this in dannye/crystal-tracker@fb01b5b
@@ -1,9 +1,9 @@ | |||
|
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.
All the png and zlib header files were re-copied straight from fltk-1.4.1/png and fltk-1.4.1/zlib.
@@ -1,6 +1,10 @@ | |||
#include <queue> | |||
#include <utility> | |||
|
|||
#pragma warning(push, 0) | |||
#include <FL/platform.H> | |||
#pragma warning(pop) |
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'm not sure these warning(push, 0)
/warning(pop)
are actually useful anymore. What do you think?
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.
@Rangi42 So FLTK (1.3 and 1.4) can produce warnings for "conversion from/to, possible loss of data" in several FL header files.
They are mostly for assigning from (int) enum to uchar.
I figured out these warnings are only triggered with <WarningLevel>Level4</WarningLevel>
in the .vcxproj file.
Level3 (which is what I've always used in crystal-tracker) doesn't produce a warning for these enum/uchar assignments.
I personally like Level3 more than Level4 in general, because I don't really care to be warned about "unreferenced formal parameter" etc.
So the options are:
- Leave the #pragmas
- Switch to Level3
- Get FLTK to add the appropriate casts in the public header files (I'll try to make this happen)
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 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.
It didn't make it into 1.4.0 but that's okay. I'll just leave the #pragmas.
@@ -53,7 +55,7 @@ Run the following commands: | |||
|
|||
```bash | |||
sudo dnf install make g++ git autoconf | |||
sudo dnf install zlib-devel libpng-devel libXpm-devel libX11-devel libXft-devel libXinerama-devel fontconfig-devel libXext-devel libXrender-devel libXfixes-devel | |||
sudo dnf install zlib-devel libpng-devel libXpm-devel libX11-devel libXft-devel libXinerama-devel fontconfig-devel libXext-devel libXrender-devel libXfixes-devel libcairo2-devel libpango1.0-devel |
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 didn't actually test this on Fedora, I just followed the convention of changing "dev" to "devel"...
Some caveats:
I haven't yet confirmed if ubuntu-latest from the github workflow already has a sufficient version of CMake installed or not.Done