Skip to content

Add fixed positioning, and the ability to set and get a component size #332

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

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

kusti8
Copy link

@kusti8 kusti8 commented Apr 3, 2018

This fixes #306 and allows me to get yoga working. So far, I've added this and the libui-node bindings as well as yoga to proton-native and it works very well.

I haven;t done any sort of work with GUI libraries really, so feel free to comment on my style. This shouldn't interfere with anything else, since all it does is add a new component. I also did this from the alpha3.5 tag because that is the latest that libui-node supported at the time. It is currently being worked on to work on the latest version.

Gustav

@kusti8 kusti8 mentioned this pull request Apr 3, 2018
ui.h Outdated
@@ -97,6 +97,8 @@ _UI_EXTERN void uiControlHide(uiControl *);
_UI_EXTERN int uiControlEnabled(uiControl *);
_UI_EXTERN void uiControlEnable(uiControl *);
_UI_EXTERN void uiControlDisable(uiControl *);
_UI_EXTERN void uiSize(uiControl *control, int *width, int *height);
_UI_EXTERN void uiSetSize(uiControl *control, int width, int height);
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, should these be names uiControlSize and uiControlSetSize, respectively?

Copy link
Owner

@andlabs andlabs left a comment

Choose a reason for hiding this comment

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

I do not agree with making the size of a control a property of the control itself. Sizes should be controlled directly by uiFixed, just as positions are; this is important as it's how all the other containers work anyway. It will also remove the need for the pointless sizing signal logic in the Unix code.

darwin/fixed.m Outdated
// 15 august 2015
#import "uipriv_darwin.h"

// TODO hiding all stretchy controls still hugs trailing edge
Copy link
Owner

Choose a reason for hiding this comment

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

Does this TODO have anything to do with this file?

Copy link
Author

Choose a reason for hiding this comment

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

No. I copied box.m and then modified that since I was unfamiliar with the layout and I forgot to take that out.

.gitignore Outdated
@@ -0,0 +1,2 @@
build
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this file.

darwin/fixed.m Outdated

// TODO hiding all stretchy controls still hugs trailing edge

@interface fixedChild : NSObject
Copy link
Owner

Choose a reason for hiding this comment

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

Use the name uiprivFixedChild.

darwin/fixed.m Outdated
- (NSView *)view;
@end

@interface fixedView : NSView {
Copy link
Owner

Choose a reason for hiding this comment

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

Use the name uiprivFixedView.

darwin/fixed.m Outdated
NSMutableArray *children;
}
- (id)initFixed:(uiFixed *)bb;
- (bool)isFlipped;
Copy link
Owner

Choose a reason for hiding this comment

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

No need to redeclare isFlipped.

darwin/fixed.m Outdated
- (id)initFixed:(uiFixed *)bb
{
self = [super initWithFrame:NSZeroRect];
self.autoresizingMask = NSViewWidthSizable | NSViewHeightSizable;
Copy link
Owner

Choose a reason for hiding this comment

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

Always use Objective-C method syntax, even for superclass property getters and setters.

Put this line in the if block.

libui uses Auto Layout; uiprivFixedView should be controllable with Auto Layout, but its contents don't necessarily have to be. I have to figure out how this would work.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think the autoresizingMask is strictly necessary, I think I just added that when trying to debug why the components weren't being sized correctly.

darwin/fixed.m Outdated
self = [super initWithFrame:NSZeroRect];
self.autoresizingMask = NSViewWidthSizable | NSViewHeightSizable;
if (self != nil) {
// the weird names vert and bb are to shut the compiler up about shadowing because implicit this/self is stupid
Copy link
Owner

Choose a reason for hiding this comment

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

No need to copy this comment over from the other files.

darwin/main.m Outdated
@@ -10,6 +10,41 @@
static BOOL (^isRunning)(void);
static BOOL stepsIsRunning;

@interface sizeWrapper : NSView {
Copy link
Owner

Choose a reason for hiding this comment

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

Why is any of this necessary? You also are not properly initializing or uninitializing instances of these objects.

Copy link
Author

Choose a reason for hiding this comment

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

This was weird to me also. When I used uiDarwinControlHandle and cast it to a NSView*, it complained about fixed not being a property. If I enclosed it in an interface that subclassed NSView, then it worked fine.

darwin/main.m Outdated
{
CGSize s = CGSizeMake(width, height);
[[[sizeWrapper alloc] addControl:control] setS:s];
}
Copy link
Owner

Choose a reason for hiding this comment

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

Please use newlines at the end of files.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Owner

@andlabs andlabs left a comment

Choose a reason for hiding this comment

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

More review comments.

unix/fixed.c Outdated
uiUnixControl c;
GtkWidget *widget;
GtkContainer *container;
GtkFixed *fixed;
Copy link
Owner

Choose a reason for hiding this comment

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

Don't use GtkFixed; it does not properly theme with CSS. GtkLayout would be better, but the ideal solution would be to have a custom GtkContainer subclass that does the barest minimum possible. Look at the version of package ui that immediately predated libui for details. (I can do this later too.)

Copy link
Author

Choose a reason for hiding this comment

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

I'll look at that.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I finally got more time to look at this. I assume you're talking about this: https://github.com/andlabs/ui/blob/de9d72299fb89a8b6cdc8963cd6b6ae708a81e80/container_unix.c
So you're overriding GtkContainer, but where do you actually set children size and location?

Copy link
Author

Choose a reason for hiding this comment

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

@andlabs How exactly would this work? I haven't done that much work with GTK.

Copy link
Owner

Choose a reason for hiding this comment

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

Line 62, which is a Go function. I forget what you call in there for each child control, but you can check the Go code.

@@ -35,6 +35,16 @@ void uiWindowsEnsureMoveWindowDuringResize(HWND hwnd, int x, int y, int width, i
logLastError(L"error moving window");
}

void uiWindowsResizeWindow(HWND hwnd, int width, int height) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm pretty sure these two are provided in winutil.cpp anyway.

Will review the rest of the code later.

Copy link
Author

Choose a reason for hiding this comment

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

There was uiWindowsEnsureMoveWindowDuringResize, but that moves and resizes the window. I added these functions because when you either move or resize a window, Windows doesn't give accurate values for the other two, so if you want it to move, it'll end up resizing also. Which is why I made these with the SWP_NOSIZE and SWP_NOMOVE flags.

Copy link
Owner

@andlabs andlabs left a comment

Choose a reason for hiding this comment

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

Missed a spot

darwin/fixed.m Outdated
return self;
}

- (bool)isFlipped
Copy link
Owner

Choose a reason for hiding this comment

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

BOOL, not bool.

@kusti8
Copy link
Author

kusti8 commented Apr 4, 2018

I've made most of the changes. As for having uiFixed control the sizing, I agree that it's a good idea, and it seems like it will work with my code so I'll change that.

darwin/fixed.m Outdated
@@ -165,7 +162,7 @@ static void uiFixedSyncEnableState(uiDarwinControl *c, int enabled)
uiDarwinControlDefaultHugsBottom(uiFixed, view)
uiDarwinControlDefaultChildEdgeHuggingChanged(uiFixed, view)

static void uiFixedChildVisibilityChanged(uiDarwinControl *c)
static void uiuiprivFixedChildVisibilityChanged(uiDarwinControl *c)
Copy link
Contributor

Choose a reason for hiding this comment

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

One "ui" too much

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I pushed so that I could check the changes in my VM. I'll fix that.

@andlabs
Copy link
Owner

andlabs commented Apr 4, 2018

Also I need to formalize some of the suggestions I made here into the contribution guidelines. Sorry if I came off as aggressive with any of them; these are just policies specific to libui to keep everything consistent!

@kusti8
Copy link
Author

kusti8 commented Apr 4, 2018 via email

@andlabs
Copy link
Owner

andlabs commented Apr 4, 2018

Also for the uipriv naming thing see #308 — we might as well start applying this to new code to make things easier when I move everything proper. utflib-and-attrstr started doing it already.

@kusti8
Copy link
Author

kusti8 commented Apr 6, 2018

Alright sizing should be moved to uiFixed now.

@kusti8
Copy link
Author

kusti8 commented May 19, 2018

Ok @andlabs I finally got a chance to implement a custom container, which I've added as fixedContainer.c. I used your old libui code and GtkFixed's source code as example which I merged, and it works well. Let me know what you think.

@kusti8
Copy link
Author

kusti8 commented May 29, 2018

@andlabs sorry for bothering you but is there anything else that I need to do? Thanks!

@kusti8
Copy link
Author

kusti8 commented Jul 8, 2018

@andlabs I know you're busy but what's the status on this?

@brunolemos
Copy link

Hopefully this can get merged so the work on kusti8/proton-native#76 can continue!

@andlabs
Copy link
Owner

andlabs commented Aug 9, 2018

I was going to write a big long expalantion of why I didn't want to merge this PR in, but thinking about it now it's been too long and I forgot exactly what I was going to say at the time. Sorry.

Here's the general gist of what my objections were:

  • the idea of the fixed layout in the first place is one I deliberately wanted to avoid having at all, because of the room for error and the amount of work it would take to make it error-free
  • I didn't want to include Yoga in libui itself as a fix for uiGrid, partially because the license is not compatible, partially because it would be a massive dependency, partially because GtkGrid, and partially because Auto Layout
  • I still don't fully understand Auto Layout — it seems like things like responding to when a basic control's intrinsic content size has changed should be something it does for us, and yet many of the bugs in uiBox and uiGrid can be boiled down to "it's not that simple"; at the same time, this PR doesn't seem to watch intrinsic content size changes at all

Of course, these original comments seem invalid now anyway, because I only see the Fixed code in the PR, not the code for any of the support APIs. What happened?

And then again, other UI toolkits have had fixed layouts alongside dynamic layouts for a while and haven't had major problems, so maybe I was just worrying too much. Perhaps a uiFixed is fine, though there's still a few things missing from libui that would allow one to be used "correctly".

I'll consider this again during the development of Alpha 5, as I want to make some major changes to uiControl itself for that (some of which were requested here).

@kusti8
Copy link
Author

kusti8 commented Aug 10, 2018

That's alright.

The original PR never included any yoga or anything like that for the exact purpose to keep libui light weight. I simply added an API for fixed position of objects. That API is used by me in JS which integrates with Yoga at that level, since yoga had a JS wrapper. Whever a change is seen by me, that is sent to yoga to recompute the coordinates and that moves everything.

@andlabs
Copy link
Owner

andlabs commented Aug 10, 2018

The ideal thing is there should be one place where you can hook into to spot control size changes. Alpha 5 will include that, since I'll be redoing uiControl entirely. This should probably also automatically mirror on an RTL system, but I have no idea where to start with that.

I'll still review this PR and see if we can put this container in.

@SkyzohKey
Copy link

Hey @andlabs @kusti8 any progress on this ? :)

@langovoi
Copy link

Any updates?

@andlabs
Copy link
Owner

andlabs commented Jan 22, 2019

The work on Alpha 5 will allow this to be feasible without encouraging the kind of mistakes that cause other toolkits (including this one) to avoid including this. It'll have to wait one more version.

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.

Fixed positioning
7 participants