#4 Adds Rounded rectangles and Borders

Open
ingles98 wil 3 commits van ingles98/master samenvoegen met pgimeno/master

I just thought these two would be good additions, at least th rounded rectangles for design's sake.

I intend on optimizing some of the clear mess, but i'd like to know if these two small features are good additions in the first place.

Anyways, thank you for maintaining the neat UI lib, so far i have been enjoying using it very much. I completely hated creating my own UI system/class so I basically gave up using my own.

I just thought these two would be good additions, at least th rounded rectangles for design's sake. I intend on optimizing some of the clear mess, but i'd like to know if these two small features are good additions in the first place. Anyways, thank you for maintaining the neat UI lib, so far i have been enjoying using it very much. I completely hated creating my own UI system/class so I basically gave up using my own.
Pedro Gimeno reageerde 5 jaren geleden
Eigenaar

This is an interesting addition, having worked on a frames theme with rounded rects myself.

Please respect the tabs for indentation. I personally hate them, but I hate even more having mixed indentation styles, so those should be fixed all at once.

EDIT: Oops, your second commit did that. Never mind that, but could you squash the commits into one? Also could you remove spaces/tabs in blank lines, and ensure that the last line ends in a newline?

I can't review the code in depth at this moment, please give me a few days.

This is an interesting addition, having worked on a frames theme with rounded rects myself. Please respect the tabs for indentation. I personally hate them, but I hate even more having mixed indentation styles, so those should be fixed all at once. EDIT: Oops, your second commit did that. Never mind that, but could you squash the commits into one? Also could you remove spaces/tabs in blank lines, and ensure that the last line ends in a newline? I can't review the code in depth at this moment, please give me a few days.
ingles98 reageerde 5 jaren geleden
Poster

Alright. I believe i fixed the identation, at least diff seems alright.

Meanwhile i will smooth things out on next commit.

Alright. I believe i fixed the identation, at least diff seems alright. Meanwhile i will smooth things out on next commit.
Pedro Gimeno reageerde 5 jaren geleden
Eigenaar

It wasn't a few days, it was a few months, apologies.

Here's my review:

  • The default value for the radius should not produce ellipses, and should not depend on the length of the side. Rounded rects in UIs have typically all corners the same radius and are typically circular. A good choice for the default would be this.style.unit / 4.
  • The new code for circle border does not restore the previous line width or style.
  • love.graphics.setColor is used. This breaks compatibility with love2d 0.10 and earlier. Use setColor (without love.graphics) instead.
  • The border is drawn at the same coordinates and with the same size as the rectangle, therefore it will extend past the box border, depending on size. This means that only even widths will appear correctly. A more sensible approach is to increase the rectangle by 0.5 pixels in every direction when the line width is odd. This will unfortunately also introduce problems at the corners when the border size is 1px, but that case has no obvious solution anyway.
  • The default for border colour should be taken from the style.
  • The rect function should draw the border too, rather than the callers, to simplify creation of custom elements. I think that the internal API can be to pass the border table instead of the mode. I don't think it needs a line mode in the first place, but since it's there, best to try to respect it.
  • When new functionality is added, the demo should include code to demonstrate it and how it's used. The "Custom Goodness" element is a good candidate for that.
It wasn't a few days, it was a few months, apologies. Here's my review: - The default value for the radius should not produce ellipses, and should not depend on the length of the side. Rounded rects in UIs have typically all corners the same radius and are typically circular. A good choice for the default would be `this.style.unit / 4`. - The new code for circle border does not restore the previous line width or style. - `love.graphics.setColor` is used. This breaks compatibility with love2d 0.10 and earlier. Use `setColor` (without `love.graphics`) instead. - The border is drawn at the same coordinates and with the same size as the rectangle, therefore it will extend past the box border, depending on size. This means that only even widths will appear correctly. A more sensible approach is to increase the rectangle by 0.5 pixels in every direction when the line width is odd. This will unfortunately also introduce problems at the corners when the border size is 1px, but that case has no obvious solution anyway. - The default for border colour should be taken from the style. - The rect function should draw the border too, rather than the callers, to simplify creation of custom elements. I think that the internal API can be to pass the border table instead of the mode. I don't think it needs a line mode in the first place, but since it's there, best to try to respect it. - When new functionality is added, the demo should include code to demonstrate it and how it's used. The "Custom Goodness" element is a good candidate for that.
Pedro Gimeno reageerde 5 jaren geleden
Eigenaar

As a side comment unrelated to the contents of this PR, it's customary to create a new branch when you're adding a feature, rather than using master, and keep master in sync with upstream. Otherwise you can't send more than 1 PR with the same repository, and you have to re-clone it.

Right now it's too late, because now the PR is tied to your master branch, but after this is closed, you can fix your repository like this:

git checkout master
git branch rounded-rect   # just to not lose it
git reset --hard 4ba024a327

and then sync master with this repo as usual.

As a side comment unrelated to the contents of this PR, it's customary to create a new branch when you're adding a feature, rather than using master, and keep master in sync with upstream. Otherwise you can't send more than 1 PR with the same repository, and you have to re-clone it. Right now it's too late, because now the PR is tied to your `master` branch, but after this is closed, you can fix your repository like this: git checkout master git branch rounded-rect # just to not lose it git reset --hard 4ba024a327 and then sync master with this repo as usual.
Pedro Gimeno reageerde 5 jaren geleden
Eigenaar

One clarification. When I say this:

The rect function should draw the border too, rather than the callers, to simplify creation of custom elements. I think that the internal API can be to pass the border table instead of the mode. I don't think it needs a line mode in the first place, but since it's there, best to try to respect it.

I mean something like this:

	rect = function(this, pos, mode)
		...
		local filltype = type(mode) == 'string' and mode or 'fill'
		local border = mode
		...
		love.graphics.rectangle(filltype, ...)
		if type(mode) == "table" then
			...
			love.graphics.rectangle('line', ...)
			...
	end
...

		this:rect(pos, this.border)
One clarification. When I say this: > The rect function should draw the border too, rather than the callers, to simplify creation of custom elements. I think that the internal API can be to pass the border table instead of the mode. I don't think it needs a line mode in the first place, but since it's there, best to try to respect it. I mean something like this: ``` rect = function(this, pos, mode) ... local filltype = type(mode) == 'string' and mode or 'fill' local border = mode ... love.graphics.rectangle(filltype, ...) if type(mode) == "table" then ... love.graphics.rectangle('line', ...) ... end ... this:rect(pos, this.border) ```
ingles98 reageerde 5 jaren geleden
Poster

Heya ! Been some time.

Yes you're absolutely right, and most of the code was going against the conventions used in Gspot. It was 'kinda' rushed heh.

Yes i know that but I created this account on notabug specifically for this and thought i'd probably only make this contrib, who know, but you're right about that. I do tend not to use master as a PR branch, sometimes i slip heh.

So i made quite a few changes that you might like. You might also wanna change things around to be honest. For instance I made border be the same shape as the object itself, but it could be a separate thing maybe.

Here's a snippet of what probably matters:

group2 = gui:group('Border test', {gui.style.unit*2, gui.style.unit * 9, 128, gui.style.unit}) -- group(label, pos, optional parent)
group2.style.fg = {255 * DIV, 192 * DIV, 0 * DIV, 255 * DIV}
group2:setshape("roundrect")
group2.style.border.color = {255 * DIV, 155 * DIV, 255 * DIV, 0 * DIV}
group2.style.border.top = 25
group2.style.border.bottom = 50
group2.style.border.right = 75

I hope you can at least find some use for it! I'd also like to help on it

Heya ! Been some time. Yes you're absolutely right, and most of the code was going against the conventions used in Gspot. It was 'kinda' rushed heh. Yes i know that but I created this account on notabug specifically for this and thought i'd probably only make this contrib, who know, but you're right about that. I do tend not to use master as a PR branch, sometimes i slip heh. So i made quite a few changes that you might like. You might also wanna change things around to be honest. For instance I made border be the same shape as the object itself, but it could be a separate thing maybe. Here's a snippet of what probably matters: group2 = gui:group('Border test', {gui.style.unit*2, gui.style.unit * 9, 128, gui.style.unit}) -- group(label, pos, optional parent) group2.style.fg = {255 * DIV, 192 * DIV, 0 * DIV, 255 * DIV} group2:setshape("roundrect") group2.style.border.color = {255 * DIV, 155 * DIV, 255 * DIV, 0 * DIV} group2.style.border.top = 25 group2.style.border.bottom = 50 group2.style.border.right = 75 I hope you can at least find some use for it! I'd also like to help on it
Dit pull-request kan automatisch samengevoegd worden.
Sign in to join this conversation.
Geen mijlpaal
Geen verantwoordelijke
2 deelnemers
Laden...
Annuleren
Opslaan
Er is nog geen inhoud.