Skip to content
This repository has been archived by the owner on Aug 29, 2024. It is now read-only.

Commit

Permalink
Regression fixes (mostly)
Browse files Browse the repository at this point in the history
- Fix #411: Regression: ProgressBar has reversed bezels...
- Fix #425: Regression: The main test app no longer clears the bg. for the "factory default" theme
- Close #427: Reinstate the ref-returning non-const getters to the SAL adapters (where they have been deleted from)!
+ Some of #429: Consistent explicit naming for accessors of the backend-facing (internal) adapters
  • Loading branch information
xparq committed Jul 16, 2024
1 parent 9d483ae commit a46b097
Show file tree
Hide file tree
Showing 12 changed files with 126 additions and 83 deletions.
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,6 @@ sfml-widgets.depend

# VS "solution" file occasionally used for debugging:
*.sln

# Misc...
.gdb_history
16 changes: 8 additions & 8 deletions include/SAL/gfx/element/ColorVertex2.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,14 @@ namespace SAL::gfx
using Impl::Impl;

// Accessors...
constexpr fVec2 position() const { return adapter()->_position(); }
constexpr Color color() const { return adapter()->_color(); }
//!! Doesn't work, but also confusing syntax: .prop() = x...:
//!! constexpr fVec2& position() { return adapter()->_position(); }
//!! constexpr Color& color() { return adapter()->_color(); }
constexpr fVec2& position() { return adapter()->_position_ref(); }
constexpr Color& color() { return adapter()->_color_ref(); }

constexpr void position(fVec2 pos) { adapter()->_position(pos); }
constexpr void color(Color c) { adapter()->_color(c); }
constexpr fVec2 position() const { return adapter()->_position_copy(); }
constexpr Color color() const { return adapter()->_color_copy(); }

constexpr void position(fVec2 pos) { adapter()->_position_set(pos); }
constexpr void color(Color c) { adapter()->_color_set(c); }


//!! Be a bit more sophisticated than this embarrassment! :)
Expand All @@ -55,7 +55,7 @@ namespace SAL::gfx
protected:
constexpr auto adapter() { return static_cast< Impl*>(this); }
constexpr const auto adapter() const { return static_cast<const Impl*>(this); }
//!!?? WTF did just `auto` here broke the const adapter()-> calls, while
//!!?? WTF did just `auto` here broke the const adapter()-> calls earlier, while
//!!?? the pretty much isomorphic Vector accessors have always compiled fine?! :-o
//!!?? Is the vector code actually incorrect (just `auto` losing `const`),
//!!?? and I've just been lucky?!
Expand Down
22 changes: 13 additions & 9 deletions include/SAL/gfx/element/ColorVertex2/SFML/Impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include "SAL/gfx/Color.hpp"

#include <SFML/Graphics/Vertex.hpp>
//#include <SFML/Graphics/Color.hpp>
//#include <SFML/System/Vector2.hpp>


namespace SAL::gfx::adapter
Expand All @@ -50,18 +52,20 @@ namespace SFML
ColorVertex2_Impl& operator = (const native_type& nv) { _d = nv; return *this; }

// Convert to SFML
operator const native_type& () const { return native(); }
operator native_type& () { return native(); }
operator const native_type& () const { return native(); }

// Accessors for the position, color, texture pos...
constexpr fVec2 _position() const { return native().position; } // fVec2 auto-converts from sf::Vector2f
constexpr Color _color() const { return native().color; }
//!! Type mismatch (SAL vs. native), but also confusing syntax: .prop() = x...:
//!! constexpr fVec2& _position() { return native().position; } // fVec2 auto-converts from sf::Vector2f
//!! constexpr Color& _color() { return native().color; }

constexpr void _position(fVec2 pos) { native().position = pos; }
constexpr void _color(Color c) { native().color = c; }
//!! Brittle binary-compatibility "exploits":
constexpr fVec2& _position_ref() { return reinterpret_cast<fVec2&>(native().position); } //!!...Add some checks (sizeof & ...)!
constexpr Color& _color_ref() { return reinterpret_cast<Color&>(native().color); } //!!...Add some checks (sizeof & ...)!

constexpr fVec2 _position_copy() const { return native().position; } // fVec2 auto-converts from sf::Vector2f
constexpr Color _color_copy() const { return native().color; } // Color auto-converts from sf::Color

constexpr void _position_set(fVec2 p) { native().position = p; } // fVec2 auto-converts to sf::Vector2f
constexpr void _color_set(Color c) { native().color = c; } // Color auto-converts to sf::Color


//!! Be a bit more sophisticated than this embarrassment! :)
Expand All @@ -75,8 +79,8 @@ namespace SFML
protected: //! Not private: meant to be mixed-in!
native_type _d;

const native_type& native() const { return _d; }
native_type& native() { return _d; }
const native_type& native() const { return _d; }
};

} // namespace SFML
Expand Down
15 changes: 11 additions & 4 deletions include/SAL/gfx/element/TexturedVertex2.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,14 @@ namespace SAL::gfx
using Base::Base;

// Accessors for texture pos...
constexpr iVec2 texture_position() const { return Impl::_texture_position(); }
//!! Doesn't work, but also confusing syntax: .prop() = x...:
//!! constexpr iVec2& texture_position() { return Impl::_texture_position(); }

constexpr void texture_position(iVec2 txpos) { Impl::_texture_position(txpos); }
//!! Can't have this: no direct access to the backend vector: it has different type! :-/ (See Impl.hpp!)
//!! Note: just leaving this enabled would take precedence over the const one! :-/
//!! constexpr iVec2& texture_position() { return Impl::_texture_position_ref(); } // _texture_position_ref() will abort if called!

constexpr iVec2 texture_position() const { return Impl::_texture_position_copy(); }

constexpr void texture_position(iVec2 txpos) { Impl::_texture_position_set(txpos); }

//!! Be a bit more sophisticated than this embarrassment! :)
static void draw_trianglestrip(const gfx::RenderContext& ctx,
Expand All @@ -38,6 +41,10 @@ namespace SAL::gfx
unsigned v_count)
{ Impl::_draw_trianglestrip(ctx, texture, v_array, v_count); } // Implicit conversion of v_array to Impl*!

protected:
constexpr auto adapter() { return static_cast< Impl*>(this); }
constexpr auto adapter() const { return static_cast<const Impl*>(this); }

}; // class TexturedVertex2_Interface


Expand Down
18 changes: 11 additions & 7 deletions include/SAL/gfx/element/TexturedVertex2/SFML/Impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "SAL/gfx/Color.hpp"

#include <type_traits> // is_same
#include <cassert> //!! Might interfere with SAL/diag. in code using this header!


namespace SAL::gfx::adapter
Expand Down Expand Up @@ -44,8 +45,8 @@ namespace SFML
/// drivers that are not able to process integer coordinates correctly.

// Copy
constexpr TexturedVertex2_Impl(TexturedVertex2_Impl&&) = default;
constexpr TexturedVertex2_Impl(const TexturedVertex2_Impl&) = default;
constexpr TexturedVertex2_Impl(TexturedVertex2_Impl&&) = default;
constexpr TexturedVertex2_Impl& operator = (const TexturedVertex2_Impl&) = default;
constexpr TexturedVertex2_Impl& operator = (TexturedVertex2_Impl&&) = default;
//!!?? Are the rval versions useful at all? Can they spare an extra copy? Wouldn't bet on it.
Expand All @@ -55,16 +56,19 @@ namespace SFML
constexpr TexturedVertex2_Impl& operator = (const native_type& nv) { Base::_d = nv; return *this; }

// Convert to SFML
constexpr operator const native_type& () const { return native(); }
constexpr operator native_type& () { return native(); }
constexpr operator const native_type& () const { return native(); }


// Accessors for texture pos...
constexpr iVec2 _texture_position() const { return iVec2((int)native().texCoords.x, (int)native().texCoords.y); } //! See the int<->float comment at the ctors!
//!! Type mismatch (SAL vs. native), but also confusing syntax: .prop() = x...:
//!! constexpr iVec2& _texture_position() { return iVec2((int)native().texCoords.x, (int)native().texCoords.y); } //! See the int<->float comment at the ctors!
constexpr iVec2 _texture_position_copy() const { return iVec2((int)native().texCoords.x, (int)native().texCoords.y); } //! See the int<->float comment at the ctors!
//!! Can't just brute-force alias this: the native type is a float vector!!!...:
constexpr iVec2& _texture_position_ref() { //!! if constexpr (!std::is_same_v<decltype(native().texCoords), sf::Vector2i>)
//!! static_assert(, "CAN'T DO THIS!..."); //!!?? ALWAYS fires, can't compile! -- Unlike in Rectangle::right/bottom! :-o
assert("DO NOT CALL _texture_position_ref()!..." && false);
static iVec2 dummy; return dummy; }

constexpr void _texture_position(iVec2 txpos) { native().texCoords = {(float)txpos.x(), (float)txpos.y()}; } //! See the int<->float comment at the ctors!
constexpr void _texture_position_set(iVec2 txpos) { native().texCoords = {(float)txpos.x(), (float)txpos.y()}; } //! See the int<->float comment at the ctors!


//!! Be a bit more sophisticated than this embarrassment! :)
Expand All @@ -79,8 +83,8 @@ namespace SFML
}

private:
constexpr const native_type& native() const { return _d; }
constexpr native_type& native() { return _d; }
constexpr const native_type& native() const { return _d; }
};

} // namespace SFML
Expand Down
2 changes: 1 addition & 1 deletion include/SAL/math/Vector.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ namespace SAL//!!::math
bool operator == (const Vector_Interface<Impl>&) const = default;
// OTOH, the meaningless <=> WOULD, so that needs to be deleted:
auto operator <=> (const Vector_Interface<Impl>&) const = delete;
// (Ironically,, op== WOULD be made automatically available from a <=>, but well...)
// (Ironically, op== WOULD be made automatically available from a <=>, but well...)

constexpr auto x() const { return adapter()->_x(); }
constexpr auto y() const { return adapter()->_y(); }
Expand Down
13 changes: 8 additions & 5 deletions include/sfw/Theme.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,24 @@ class Theme
// Quick-And-Dirty Theme Config Pack
// -- will gradually encompass the entire Theme class,
// by moving from static Theme data to config objects...
// This is the initial version to support the Demo app.
// This is the initial version, just to support the demo/test apps.
struct Cfg
{
// Note: the ordering is optimized for init lists like
//
// { "asset/custom", "MyClassic", "mytextures.png", Color("#e6e8e0"), 12, "font/MyFont.ttf" }
// { "asset_dir/custom", "MyTheme", "mytextures.png", Color("#e6e8e0"), 12, "font/MyFont.ttf" }
// { "asset_dir/custom", "MyTheme", "mytextures.png", Color("#e6e8e0"), 14 } // Default font, but larger
//
// Note: these are not "defaults" here, but "unset" markers (and/or diagnostic sentinels)
// (except when the type doesn't allow such "off-band" values; then it's indeed a default).
const char* name = nullptr;
const char* basePath = nullptr;
const char* textureFile = nullptr;
Color bgColor = Color::None; // "transparent black": closest to an unset "null" color value
Color bgColor = 0xBadC0100; // Not using (reserving!) Color::None here as the unset ("null") value,
// to allow using it for "no background color" in a theme config!
// See #425 for (much ;) ) more!
Wallpaper::Cfg wallpaper = {};
size_t textSize = 0; //!!?? uint16_t: a) fixed (-> ABI compat.!), b) smaller, c) but may need annoying extra casts, d) uint_least16_t is the guaranteed type actually...
size_t textSize = 0; //!! #424 (Rename), #426 (Stop using size_t for trivially small things)
const char* fontFile = nullptr;
bool multiTooltips = false;

Expand Down Expand Up @@ -94,7 +97,7 @@ class Theme
static Style click;
static Style input;

static size_t textSize;
static size_t textSize; //!! #424 (Rename), #426 (Stop using size_t for trivially small things)

static Color bgColor;
static Wallpaper::Cfg wallpaper;
Expand Down
20 changes: 13 additions & 7 deletions include/sfw/widget/ProgressBar.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "sfw/geometry/Orientation.hpp"
#include "sfw/gfx/element/Box.hpp"
#include "sfw/gfx/element/Text.hpp"
#include "sfw/gfx/element/TexturedVertex2.hpp"


namespace sfw
Expand Down Expand Up @@ -79,15 +80,20 @@ class ProgressBar: public Widget
float track_length() const;
float val_to_barlength(float v) const;

enum VertexIndex : unsigned { TopLeft, BottomLeft, TopRight, BottomRight, _VERTEX_COUNT_ };
enum : unsigned {
//!! Reuse these from... somewhere...:
TopLeft, BottomLeft, TopRight, BottomRight,
_VERTEX_COUNT_
};

// Data
Cfg m_cfg;
float m_value;
Box m_box;
sf::Vertex m_bar[_VERTEX_COUNT_];
gfx::Text m_label;
}; // ProgressBar
Cfg m_cfg;
float m_value;
Box m_box;
gfx::TexturedVertex2 m_bar[_VERTEX_COUNT_];
gfx::Text m_label;

}; // class ProgressBar


//----------------------------------------------------------------------------
Expand Down
File renamed without changes.
55 changes: 33 additions & 22 deletions src/lib/Theme.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,20 @@ namespace sfw
{
using namespace geometry;


Theme::Cfg Theme::DEFAULT =
{
.name = "",
.basePath = "asset/",
.textureFile = "texture/default.png",
.bgColor = Color::White,
.wallpaper = {},
.textSize = 12,
.fontFile = "font/default.ttf",
.multiTooltips = false, //! Ignored! The desig. init. value can't be made distinguishable
//! from an artificial "use default" inline member-init val. in Cfg::.
};

//============================================================================
// BEGIN class Theme::Cfg
//============================================================================

bool Theme::Cfg::apply()
{
// Pick up some defaults first...
// (This really should be a constructor, but that would piss off C++ and make it reject the designated inits. :-/ )
if (!name) name = Theme::DEFAULT.name;
if (!basePath) basePath = Theme::DEFAULT.basePath;
if (!fontFile) fontFile = Theme::DEFAULT.fontFile;
if (!textureFile) textureFile = Theme::DEFAULT.textureFile;
if (textSize <= 1) textSize = Theme::DEFAULT.textSize;
// (This really should be a constructor, but that would piss off C++, making it reject desig. init.! :-/ )
if (!name) name = Theme::DEFAULT.name;
if (!basePath) basePath = Theme::DEFAULT.basePath;
if (!fontFile) fontFile = Theme::DEFAULT.fontFile;
if (!textureFile) textureFile = Theme::DEFAULT.textureFile;
if (textSize <= 1) textSize = Theme::DEFAULT.textSize;
if (bgColor == 0xBadC0100) bgColor = Theme::DEFAULT.bgColor; // See #425 for that code choice!

// Update the global (`static`) Theme data...

Expand All @@ -44,11 +34,11 @@ bool Theme::Cfg::apply()

if (string path = string(basePath) + fontFile; !Theme::loadFont(path))
{
return false; // SFML has already explained the situation...
return false; //!! SFML has already explained the situation...
}
if (string path = string(basePath) + textureFile; !Theme::loadTexture(path))
{
return false; // SFML has already explained the situation...
return false; //!! SFML has already explained the situation...
}
Theme::textSize = textSize;

Expand All @@ -59,6 +49,27 @@ bool Theme::Cfg::apply()
}

//============================================================================
// END class Theme::Cfg
//============================================================================


//============================================================================
// BEGIN class Theme
//============================================================================

Theme::Cfg Theme::DEFAULT =
{
.name = "",
.basePath = "asset/",
.textureFile = "texture/default.png",
.bgColor = Color::White,
.wallpaper = {},
.textSize = 12,
.fontFile = "font/default.ttf",
.multiTooltips = false, //! Ignored! The desig. init. value can't be made distinguishable
//! from an artificial "use default" inline member-init val. in Cfg::.
};

Theme::Cfg Theme::cfg; //!! Mostly unused yet, but slowly migrating to it...

size_t Theme::textSize = Theme::DEFAULT.textSize;
Expand Down
Loading

0 comments on commit a46b097

Please sign in to comment.