nk_widget_text: use left alignment by default instead of silent failing + small refactor

[sleeptightAnsiC]

Calling any function that depends on nk_widget_text without providing X-axis alignment flag (e.g. nk_label(ctx, "foo", 0); ) would silently fail and result in no text displayed at all.

For comparison, Y-axis is not affected by this issue and simply defaults to TOP. Other widgets are also not affected, only Text is.

This PR fixes the issue by making sure that X-axis defaults to LEFT (see 1st commit).

Additionally, I made sure that branches for X and Y axis look the same (see 2nd commit).

There are some more details in commit messages.

[sleeptightAnsiC]

I removed this line because label.x and label.w is always set inside of every x-axis branch, and now none of these branches can be missed, so this line has no effect.

[PavelSharp]

Hi, thanks for tackling this annoying issue. I think the approach with checking all horizontal (and vertical) alignment flags works, but it could be simplified. Instead of explicitly testing for each horizontal alignment and then setting a default, we could achieve the same behavior more directly. Let me show. Instead of:

   if (!(a & (NK_TEXT_ALIGN_LEFT | NK_TEXT_ALIGN_CENTERED | NK_TEXT_ALIGN_RIGHT)))
        a |= NK_TEXT_ALIGN_LEFT;
   if (a & NK_TEXT_ALIGN_LEFT) 
        ...
   else if (a & NK_TEXT_ALIGN_CENTERED)
        ...
   else if (a & NK_TEXT_ALIGN_RIGHT)
        ...

We can achieve this behavior in a more direct way:

   if (a & NK_TEXT_ALIGN_CENTERED)
        ...
   else if (a & NK_TEXT_ALIGN_RIGHT)
        ...
   else //NK_TEXT_ALIGN_LEFT as default
   {
        NK_ASSERT(a & NK_TEXT_ALIGN_LEFT && "Unknown alignment flag detected")
        label.x = b.x + t->padding.x;
        label.w = NK_MAX(0, b.w - 2 * t->padding.x);     
   }          

And of course same logic for vertical flags

[sleeptightAnsiC]

@PavelSharp Thanks for feedback.

I believe that current explicit assignment+tests is a slightly better approach, because the default value can be changed by only changing one line instead of messing with branches itself, and because you don't really need comments to describe what's going on. Also, I don't like relaying on the default/else branch semantic, since it often breaks whenever you change something related.

However, your approach might be more idiomatic when compared to the rest of Nuklear, so I will take a look at similar code paths elsewhere, whenever I have some spare time.

[PavelSharp]

@sleeptightAnsiC
Thanks for the explanation, I see your point, and that makes sense. My approach was mainly about avoiding the need to check all alignment flags twice, while your version has the advantage of making the default behavior more explicit. Either way, these are minor details.

It might be worth clarifying whether arbitrary or invalid flag combinations (for example NK_TEXT_ALIGN_LEFT | NK_TEXT_ALIGN_CENTERED) should be accepted, or if only the predefined ones plus 0 are considered valid defaults. In the latter case, adding an NK_ASSERT could help make this expectation more explicit.

Something similar to this:

int x = a & (NK_TEXT_ALIGN_LEFT | NK_TEXT_ALIGN_CENTERED | NK_TEXT_ALIGN_RIGHT);
NK_ASSERT(x == 0 || (x & (x - 1)) == 0);

[sleeptightAnsiC]

@PavelSharp This would be nice, but it's too much for this PR.

When adding such checks here, I should also add them in any other similar code paths that use the same flags, right? Also, this could be a breaking change, if someone depends on the buggy behavior of passing multiple self-overriding flags, and would require additional testing. Let's stick to fixing one problem in nk_widget_text at a time 😅

So this is out of the scope for now, but definitely worth adding someday.

[PavelSharp]

@sleeptightAnsiC
My initial motivation for writing that comment came from a general approach to work. When I start improving something, I like to bring it to a fully polished state. So, since we began improving nk_widget_text, it felt natural to also handle incorrect flag combinations as part of making it complete.

I’m sorry if my message sounded like I was suggesting changes across all similar places. I was only referring to this function.

That said, I now agree it’s probably better to leave the flag validation aside for now, since we don’t have enough information to tell whether this behavior is an oversight or an intentional part of Nuklear’s design.

Either way, I hope your PR gets merged soon😇