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

Possibly fix multiline labels on windows #112

Merged
merged 5 commits into from
Jul 18, 2022

Conversation

mohad12211
Copy link
Contributor

@mohad12211 mohad12211 commented Jun 18, 2022

unix and darwin appear to have no problems with multiline labels (would like confirm that).
this is an attempt to solve that on windows and address #106.
the first commit from andlabs/libui#465 to calculate the correct width and solve #106

Screenshot and code example
#include <stdlib.h>
#include <string.h>
#include "../../ui.h"

int onClosing(uiWindow *w, void *data)
{
	uiQuit();
	return 1;
}

int main(int argc, char *argv[])
{
	uiInitOptions o;
	memset(&o, 0, sizeof (uiInitOptions));
	if (uiInit(&o) != NULL)
		abort();
	uiWindow *w = uiNewWindow("Hello", 100, 500, 1);
	uiBox *hbox = uiNewHorizontalBox();
	uiWindowSetChild(w, uiControl(hbox));
	uiButton *b = uiNewButton("TEST");
	uiLabel *l = uiNewLabel("1\n\n\n\n\n1\n\n\n\n\n1\n\n\n\n\n1\n\n\n\n\n1\n\n\n\n\n1\n\n\n\n\n1");
	uiBoxAppend(hbox, uiControl(b), 1);
	uiBoxAppend(hbox, uiControl(l), 1);
	uiWindowOnClosing(w, onClosing, NULL);
	uiControlShow(uiControl(w));
	uiMain();
	return 0;
}

before (smallest window width):
image
after:
image

second commit is to calculate the correct height based on how many lines will be in the label.

Screenshot and code example

if we change the label in controlgallery to:

uiControl(uiNewLabel("This is a label.\nRight now,\nlabels can\nonly span\none line.")),

before change:
image

after change:
image

This only addresses uiLabel, what about uiForm? does it make sense to have a form with multiline label?

Copy link
Contributor

@cody271 cody271 left a comment

Choose a reason for hiding this comment

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

This is comparing an ASCII literal against UTF-16 strings to parse the newlines. Otherwise looks good.

windows/text.cpp Outdated Show resolved Hide resolved
windows/text.cpp Outdated Show resolved Hide resolved
windows/text.cpp Outdated Show resolved Hide resolved
@cody271
Copy link
Contributor

cody271 commented Jun 24, 2022

unix and darwin appear to have no problems with multiline labels (would like confirm that).

Confirmed to be a Windows only issue, correct.

Screenshot and code example

This always makes reviewing much easier, thanks! 👍

Maybe now also update that label text in the controlgallery example to reflect the fact that we do support multiline labels on all platforms? ;)

This only addresses uiLabel, what about uiForm? does it make sense to have a form with multiline label?

I wouldn't worry about supporting multiline labels in forms, no.

Copy link
Contributor

@szanni szanni left a comment

Choose a reason for hiding this comment

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

Nice work, definitely seems to work according to my tests.

Apart from removing out the old, now unneeded cruft, please remove the curly braces around the single if statements. The general guideline is to add them only when nesting gets too crazy or to match if/else blocks.

And please add the missing spaces around if () and while () statements. You did it in some places but not others.

Edit: and all the things @cody271 suggested of course.

windows/text.cpp Outdated Show resolved Hide resolved
windows/text.cpp Outdated Show resolved Hide resolved
windows/text.cpp Outdated Show resolved Hide resolved
windows/text.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@szanni szanni left a comment

Choose a reason for hiding this comment

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

Missed the memory leak in my first review.

windows/text.cpp Show resolved Hide resolved
@mohad12211 mohad12211 requested a review from cody271 July 12, 2022 02:35
Copy link
Contributor

@szanni szanni left a comment

Choose a reason for hiding this comment

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

Very nice. Code looks good to me.

I tested things as well under Window 7 with assistive technologies, as I was a little concerned about the hard coded line height. Results:

100% font/icon size:
small

150% font/icon size:
big

It seems like Windows actually takes care of things via some type of smart scaling. So all good it seems.

@cody271 cody271 merged commit d907bd3 into libui-ng:master Jul 18, 2022
@mohad12211 mohad12211 deleted the multi-line-label branch July 18, 2022 10:07
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.

4 participants