xparq / sfw Goto Github PK

-> https://github.com/xparq/sfw/actions/runs/4264438391/jobs/7422515075

Run clang-format $(find src/impl -type f) --dry-run -Werror


src/impl/SpriteButton.cpp:72:22: error: code should be clang-formatted [-Wclang-format-violations]
    sf::Vector2i size{(int)getSize().x, (int)getSize().y};
                     ^
src/impl/Slider.cpp:20:2: error: code should be clang-formatted [-Wclang-format-violations]
}
 ^
src/impl/Menu.cpp:75:59: error: code should be clang-formatted [-Wclang-format-violations]
        if (gui::Theme::cursor.loadFromSystem(cursorType)) {
                                                          ^
src/impl/Menu.cpp:75:1: error: code should be clang-formatted [-Wclang-format-violations]
        if (gui::Theme::cursor.loadFromSystem(cursorType)) {
                                                            ^
src/impl/Menu.cpp:6:54: error: code should be clang-formatted [-Wclang-format-violations]
                m_window.setMouseCursor(gui::Theme::cursor);
                                                            ^
src/impl/Menu.cpp:77:36: error: code should be clang-formatted [-Wclang-format-violations]
                m_cursorType = cursorType;
                                          ^
src/impl/TextBox.cpp:404:15: error: code should be clang-formatted [-Wclang-format-violations]
    glScissor((GLint)(pos.x + Theme::borderSize), (GLint)(target.getSize().y - (pos.y + getSize().y)),
              ^
src/impl/TextBox.cpp:404:103: error: code should be clang-formatted [-Wclang-format-violations]
    glScissor((GLint)(pos.x + Theme::borderSize), (GLint)(target.getSize().y - (pos.y + getSize().y)),
                                                                                                      ^
src/impl/TextBox.cpp:405:36: error: code should be clang-formatted [-Wclang-format-violations]
              (GLsizei)getSize().x, (GLsizei)getSize().y);
                                   ^
src/impl/TextBox.cpp:405:57: error: code should be clang-formatted [-Wclang-format-violations]
              (GLsizei)getSize().x, (GLsizei)getSize().y);
                                                        ^
src/impl/Theme.cpp::51: error: code should be clang-formatted [-Wclang-format-violations]
    if (!cursor.loadFromSystem(sf::Cursor::Arrow)) {
                                                  ^
src/impl/Button.cpp:64:2: error: code should be clang-formatted [-Wclang-format-violations]
{x, y; // <- This can only silence CL -W4, but not g++ -Wall though... :)
 ^
Error: Process completed with exit code 1.

I've actually done this once:

auto child = box->addVBoxLayout();
// Use child:
// ...lots of child->add(...); calls...
// etc.
box->add(child); //! Oops: infinite loop! :-o

Also happens in the original!

And, probably due to the same underlying problem: clicking in the edit field and then quickly removing the mouse makes it lose the focus.
-> UPDATE: No. That was #141, and this stuck cursor happens even after fixing that.

Just a simple whitespace skipper for a start.

(Follow-up of #20.)

Looks fine in the original. I remember seeing multiple SFML comments about overly antialiased texts, but no idea if any of that is relevant -- the other ("static") texts are just fine after all!
-> Well, not, not all the other texts are fine, either: e.g. button labels, depending on (font?) size etc...

Also: the OptionsBox arrows are "dirty"... :-o And not sure why they're smaller than the original, too... -> #97 (Closed now.)

And: Slider int/float sloppiness... -> #47 (Closed now.)

The only thing so far is the post-fork-baseline tag.
-> Now not even that, as it got promptly deleted for "changed my mind" reasons. :)

-> #237

Note: the GHA builds just worked on the GH Ubuntu runner!

linking executable sfw-demo
/usr/bin/ld: warning: libXcursor.so.1, needed by extern/sfml/lib/libsfml-window.so, not found (try using -rpath or -rpath-link)
/usr/bin/ld: ./lib/libsfw.a(TextBox.o): in function `gui::TextBox::TextBox(float)':
TextBox.cpp:(.text+0x14ad): undefined reference to `sf::Clock::Clock()'
/usr/bin/ld: extern/sfml/lib/libsfml-window.so: undefined reference to `XcursorImageLoadCursor'
/usr/bin/ld: extern/sfml/lib/libsfml-graphics.so: undefined reference to `__libc_single_threaded@GLIBC_2.32'
/usr/bin/ld: extern/sfml/lib/libsfml-window.so: undefined reference to `XcursorSupportsARGB'
/usr/bin/ld: extern/sfml/lib/libsfml-window.so: undefined reference to `XcursorImageCreate'
/usr/bin/ld: extern/sfml/lib/libsfml-window.so: undefined reference to `XcursorImageDestroy'
/usr/bin/ld: extern/sfml/lib/libsfml-graphics.so: undefined reference to `std::__throw_bad_array_new_length()@GLIBCXX_3.4.29'
collect2: error: ld returned 1 exit status
make: *** [Makefile:20: sfw-demo] Error 1

-> https://www.sfml-dev.org/tutorials/2.5/compile-with-cmake.php

In addition to #39:

• rename Utils to lowercase, and something singular; also distinguish from possible other "utils" that have nothing to do with gfx

• Layouts -> Layout (as a module name)

• rename Enums or delete it + move its sole header to that "possible other utils" if created...

• Gui.hpp -> (../?)SFW.hpp

• and of course Menu.hpp -> #6

• reverse #include order in Theme.hpp? It's actually the preferred order e.g. by https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

• Widget.hpp (and possibly others): swap
  #include <SFML/System/Vector2.hpp>
  #include <SFML/Graphics/Drawable.hpp>

It seems to be a bog standard ImageButton, actually.

Or, better yet: have both. Fork it to a floating widget (add that style then first (to Layout?!), created/updated from a sprite.
-> Well, OK, no. :) Maybe later, if there's nothing left to do.

Obviously, also to be configurable via widget styles (#11).

-> E.g. https://forum.qt.io/topic/20191/how-to-make-a-slider-widget-with-more-than-one-slider-and-set-color-of-line/10

BTW, do get the GHA cmd use the tooling/lint.sh!

Like OptionsBox arrows. And CheckBox... And Slider... (Upstream also has this problem!) See also #141!

• 1. Hover events tend to get ignored/lost, when entering a container, and then a widget within that container! (The widget is clearly not in the Hovered state, according to the missing blue debug insights outline.) Slow mouse moves tend to avoid that.

  • OK, see fix in a later comment!
    But what about that "priming" effect (hopefully described somewhere below)?!

• 2. Even worse: sometimes the hover rect is there, and yet the clicks appear to not be propagated! :-o
  Couldn't reproduce it recently though (2023 July). I tend to believe that it was either a mistaken observation, or some transient glitch that is no more.

Can this be actually losing those events, due to the usual SFML inner polling loop to eat all pending ones in one frame?!
Because last time this was really noticeable under high load!
-> Unfortunately, this seems to be the case without load, too. :-/

https://blog.rubenwardy.com/2020/01/21/creating-game-guis-in-sfml/

BTW, FSGUI uses PascalCase, FWIW. That's a nice "data point"... And it has a nice plugin Renderer approach!

Ruben Wardy (above): "TGUI feels badly designed and incomplete."

• Adjust the demo & test app, too!
• And then auto-calibrate the stepping... -- delegated to #288!

There are some int/float mixes in the Theme params (peaking at int Theme::borderSize = 1.f; in Theme.cpp), but a bigger nuisance is the int->float conversions in the Utility classes due to e.g. Theme::getTextureRect (FloatRect vs. IntRect).

Anyway, try to introduce unsigned short for size types (and also for all widget coords?!), like e.g. for:
Theme::borderSize // int now
Theme::minWidgetWidth // int now

But it may actually yield even more warnings then for signed/unsigned mismatches (esp. if widget screen coords end up signed)... :)

Some other findings (from #3):

• [DONE] These really shouldn't be mismatches (-> demo.cpp)!

    pbarRotation1->setValue(sliderRotation->getValue());
    pbarScale1->setValue(sliderScale->getValue());
    pbar->setValue(vslider->getValue());
  

• Lots of setCharacterSize((unsigned)Theme::textSize)) and similar... -> would Theme::textSize be better just unsigned?

• (float)Theme::getBoxHeight() ditto.

• Theme::PADDING and ::MARGIN are floats. Why those? (I can't recall now how much, but as if those needed some conversions, too.)

(Also on a mission to fight the trend of everyone calling every non-editable list box "combo" (started by the Dear IMGui guy?)... ;) )

For a classic reference: https://learn.microsoft.com/en-us/windows/win32/Controls/about-combo-boxes

If I'm at it anyway (#20):

• Streamline TextBox::setText
• rid cursor comment from ctor

Also in upstream.

From the orig. README (ignore the typo):

gui::Menu
The special, unique root layout. It behave like a VBoxLayout."

So... Where are the real menus (pulldown, context etc.) then?...

ChatGPT's best suggestions, after a long massage session (as a baseline ref., and to make sure I'm not missing something obvious):

GuiCtrl
WidgetMgr
UiHandler
GraphCtrl
DisplayCtrl
GuiMaster
ScreenCtrl
WindowMgr
UiMaster
VisualCtrl

(And something "Session"-ish was also one I've been contemplating...)

Also, if the new namespace could be kept sg. descriptive (other than just "sfw"), it could help...

Also traits beyond theming (i.e. behavior).

Like for Button: trigger-on-mouse-press, trigger-on-key-release etc.)
Should be bitfields, with preconfig. packs also selectable (as usual).

Of course, they don't have to be specific, some could as well go to Widget, if applicable there.

Also reconcile with the two existing input widget styles of "textual" and "...the other" -> #308... :)

Start with the most visible ones: ProgressBar...

Namely, tried with OptionsBox, which did allow copy, but its clone misbehaves...
-> NO, FALSE ALARM: it seems to work now... BUT! Some other bug does exist that sometimes prevents it from reacting to mouse clicks! :-o I have to nudge the mouse to start reacting again! :-o -> #45

Anyhow: copying callbacks makes no sense, until shifting to #27 with a this parameter!

gui::Button* button = new gui::Button("My button");
button->setCallback([] {
    std::cout << "click!" << std::endl;
});
gui.add(button);

->

gui.add(new gui::Button("My button", [] {
    std::cout << "click!" << std::endl;
}));

Basically detach the lib interface into an include dir (with a package "tag" subdir, like in FSML, since we're in the FSML club now).

Also: undo my change to put the lib in out. The original lib dir was a better choice after all.

(Like "multi-arch" on Linux etc.)

This should in turn finally enable local build (i.e. linking! -- compilation is platform-agnostic already!) with both MSVC and GCC without any additional SFML setup when switching between them.

A regression after I tried silencing the warnings of the int -> float conversion chain in the ctor by starting out with floats...
If I leave the original, the widgets all look fine, but there're 10 warnings to tame. :)

It' basically that size was int, so going for short instead did the trick.
But then I (mostly) ended up with unsigned short, as that's a better abstraction there.

Also changed the API from int to float, to

• be more flexible (preparing to arbitrary ranges later: #49), and to
• get rid of the nasty inconsistency causing int/float conv. warnings for pbarScale->setValue(sliderScale->getValue()) in the demo (see #4, #3).

I was too eager to remove dependencies (#9), and may have removed too much of it...

A quick look suggests I haven't done it actually... (But looking thru the peephole of my phone now; should check properly.
OK. Done; none...)

But keep the current setCallback API for use cases without inheritance, of course.

So, this would allow finer granularity of callbacks: the existing ones on the instance level, plus one on the class level.
Derived widgets that override onUpdate could make some explicit callback assignments (to individual widget instances) unnecessary, but what's more important is the more consistent, unified feel of an onUpdate method -- which kinda looks more intuitive than it actually is! :) (But with the deeper understanding required for realizing that it's not quite exactly what its name initially suggests, one would probably have no problem adjusting mentally...)

Also, kinda unrelated, but a parallel (and similarly special), but generic catch-all virtual Widget::onEvent() + ->setEventCallback() API could be useful, too. (But with the usual annoying complications of who and when should (not) call which one then etc.)

So, e.g.:

void Widget::triggerUpdateCallback()
{
    onUpdate(); // call the class-level (virtual) callback

    if (m_updateCallback)
    {
        m_updateCallback();   // call the instance-level callback
    }
}

Or just:

void onUpdate() // Class-level overrides should also call back to this explicitly as Widget::onUpdate()!
{
    if (m_updateCallback)
    {
        m_updateCallback();   // call the instance-level callback
    }
}

Forgot to finish porting?

BTW, where is that called, actually?

-> onMouse**Released** and onKey**Pressed** -- Should they be consistent on Release?
That (onKeyRelease) would prevent reacting on auto-repeated keypresses, though. Which may or may not be desired... -> #11

-> e11284e

Demo the new features, too.

Should itself be a Widget, too? Or at least Event::Handler, to receive events (see below)... Where in the widget tree anyhow? Should obviously be a "child" of their target widget "somehow" (or "in some sense") -- but should that warrant making Widget itself be a WidgetContainer already (could simplify a bunch of things anywa...), or would this be a different kind of (2-node hierarchy) relationship? Let's call it "ownership" then. The important thing is that they should "kinda depend on" their (one) owner widget.

Also: "singletons"?? Can't recall ever having more than one at once... (They certainly are for one widget! :) )
I see no compelling reason for limiting them to only one, though. Why force a wait to allow showing another??

It's a 3-phase process (each configurable):

1. Pre-delay: let the user linger on a widget doing nothing for a while, without a tooltip immediately popping up
2. Sustain: keep it up for this long; should depend on text length and a configurable rate
3. Fadeout

Note:

• Phase 1 should start on MouseEntered of the owner -- but probably not if already focused --, and phase 2 if idling there for the pre-delay time (i.e. no events during that). Any other event -- except MouseMoved -- should dismiss the tooltip. MouseMoved should just do nothing (but that might change!...).
• A dismissed tooltip should not reappear until reset by a MouseLeft event. (And possibly others?...)
• Hovering over them should keep resetting the fadeout timers. (But not their initial delay!)
• Clicking it should close it immediately. (But then this would prevent copy-pasting from it! So maybe a double-click should, instead. Or, a right click should be used for copying + any other contextual ops. that may come up relevant.)
• It should also be configurable how (e.g. less frequently? after longer wait?) tooltips reappear for widgets they've already been shown for.
• Disabled widgets should have their tooltips disabled, too.

For all this logic to actually work transparently (without messing with all the derived widgets), it can't be done just by the widget event callbacks: it must be handled by the thing that calls those! (Which is Layout currently.)

And just for ref.: https://learn.microsoft.com/en-us/windows/win32/Controls/individual-control-info

Failed get() should also point to a dummy default widget to prevent nullref bugs... (Kinda like how std::map["missing"] never fails; and indeed, an std::map would back this feature.) Actual error feedback/checks could be added by alternative ways.
-> UPDATE: Well, get can't just indiscriminately return a safe default, if there's really no such widget... Instead, it should leave this choice for the user, and mirror the std::map interface: provide both an "easy, but approx." and a "tedious, but precise" interface!

NOTE: #34 (method chaining) would make this largely unnecessary, but not completely: some methods would still need to be called later on -- e.g. by other widgets --, and would be nice to do that, too, without var. assignments. Also, it's still invaluable for diagnostics.

Unfortunately, Quads are now gone. Must rework the shape structures to use TriangleStrip (or just triangles).
BTW, it's not just the vertices in Box/Cross, but quite a few similar places in some widgets, too!

This might help... (If not, still looks like a good reading. ;) )

-> a6370c0, 1011152 etc.

Just bookmarking this, has some nice (possibly relevant) answers -> https://stackoverflow.com/questions/9242234/c11-variadic-stdfunction-parameter

Up/Down is currently used for that. The fact that support exists at all, is very nice. However, it's a bit counterintuitive, as it has nothing to do with the vertical direction actually. It's also inconsistent with the totally different uses of Left/Right. And it will be needed for other uses, too, later, like smarter control of sliders depending on orientation, or multi-line edit boxes, of course, if they get implemented one day.

(And -pedantic and all the rest also set in Alex's original Makefile.)

Phew, OK, MSVC and GCC are clean now (except for some leftover float->int in demo.cpp, but c'est la vie).

Also the Box, Cross, Arrow, Menu hpps!

Note that with SFML 3, even mere class predeclarations should suffice!
-> Which could foster even an overall (gradual) distancing of the entire lib (primarily its interface) from SFML in general!
-> NO! Forward decl. instead of headers is a VERY BAD IDEA! (-> #39)

(Also in upstream.)

Nothing, literally. :) It's just being loaded, but never used/drawn.

At least the existing void methods could do it for free already.
Still not much use until #27 though... (to spare the creation of a temp. local Widget* if the callback would actually use the widget...).