Adds tab and padding panel #21058

goddessfreya · 2017-05-19T23:43:59Z

Observable Changes

Tab's have become slightly more compact

Implementation details

Moved code for handling window size and windows to ui::window
(note: I did not change the contents of the tabs to use the new system as that needs other panels)
Moved code for creating tabs and choosing what goes in them to ui::tab_panel
(note: currently the tabs don't use the tab_pane)

Adds tab and padding panel.

Ready for review.

Signed-off-by: Hal Gentz <[email protected]>

Leland · 2017-05-20T05:23:36Z

Thanks for tackling accessibility! It's an unsexy and often overlooked part of development. Considering how large of a range and how far-reaching a change you're looking to eventually cover with this PR, it would probably make things much easier on the review team if you broke this out in a couple different PRs – your outline makes planning that pretty easy. E.g. the first PR could implement the basic concept, and extend it to one instance.

This will also ensure that each building block of your eventual, complete PR will be approved at a granular level, rather than submitting the whole thing and needing to potentially revise large blocks of code to fit the ensuing review process.

I'd probably recommend working on a fork in your branch until that first, building block of a PR is approaching a point where it can be reviewed. Excited to see how this turns out!

ZhilkinSerg · 2017-05-20T07:14:31Z

Will navigation keys be hardcoded or will it be possible to remap them?

kevingranade · 2017-05-20T07:16:39Z

A few organizational notes:
Please proceed in a depth-first fashion here, i.e. implement a widget and add it to the game as a standalone pr, then create the next widget and integrate it, etc. If this ended up as one giant PR, it would be at a very high risk of never being merged.
Take a look at the projects feature, if you're interested in using it i think it would be a great fit for this project. (I'll need to grant you some permissions to do it, just let me know if you're interested)

goddessfreya · 2017-05-20T07:29:18Z

@ZhilkinSerg Hopefully remapable.

@Leland @kevingranade Ok, I've updated the goals for this PR to be something simpler.

@kevingranade I'd love to have Project Features, they would be nicer than a huge checklist.

Signed-off-by: Hal Gentz <[email protected]>

Scince I haven't moved anything to my panels I have zero size! High quality work here! Signed-off-by: Hal Gentz <[email protected]>

BevapDin · 2017-05-20T10:25:19Z

src/UIHandler.cpp

+}
+
+void UIParentPanel::addChild(UIPanel *panel)
+{   if (m_childPanels.size() != 0)


Why don't you use empty() instead of comparing the size?

I did not know of the empty function, thanks.

BevapDin · 2017-05-20T10:26:29Z

src/UIHandler.h

+#include "cursesdef.h"
+#include "output.h"
+
+struct IntPair


We already have a class for this. It's called point.

Thanks once again.

BevapDin · 2017-05-20T10:27:26Z

src/UIHandler.cpp

+
+IntPair UIParentPanel::RequestedSize(bool min)
+{
+    IntPair size;


Could be shorter: IntPair size = { 2, 2 };

BevapDin · 2017-05-20T10:27:47Z

src/UIHandler.cpp

+    size.x = 2;
+    size.y = 2;
+
+    if (m_childPanels.size() == 0)


Again: you want empty() here.

BevapDin · 2017-05-20T10:28:23Z

src/UIHandler.cpp

+
+    auto paneSize = m_childPanels[0]->RequestedSize(min);
+    size.x += paneSize.x;
+    size.y += paneSize.y;


The point class allows to write this in one command: size += paneSize;

BevapDin · 2017-05-20T10:40:22Z

src/UIHandler.h

+    virtual void addChild(UIPanel *panel) = 0;
+    virtual void removeChild(size_t index) = 0; // Passes back removed child
+
+    virtual IntPair RequestedSize(bool min) = 0;


I don't get the meaning of the parameter. bool parameters are always suspect. It's probably better to make this a private function and have two separate public function that call it:

IntPair RequestMinSize() const { return RequestSize(true); } IntPair RequestMaxSize() const { return RequestSize(false); }

Even better would be an enum, because nobody will remember whether the bool means min or max (whether RequestSize(true) returns the minimal size or the maximal size). Or does it return the true and the untrue size, whatever that means?

enum Foo { // find better name MININAL, MAXIMAL }; void RequestSize(Foo f);

The parameter is for getting either a the minimal size or the preferred size. It probably should be an enum.

BevapDin · 2017-05-20T10:42:43Z

src/UIHandler.h

+    {
+        Centered,
+
+        Undefined = 0,


Is it on purpose that Undefined and Centered have the same value? If so, you might want to make this explicit: Undefined = Centered,.

BevapDin · 2017-05-20T10:43:49Z

src/UIHandler.h

+    UIWindow(int minSizeX, int minSizeY, Location location);
+    int UpdateWindow();
+private:
+    UIPanel* m_panel = new UIParentPanel();


Memory leak. This is never freed. That's where smart pointers help a lot.

BevapDin · 2017-05-20T10:45:41Z

src/UIHandler.h

+class UIPanel
+{
+public:
+    virtual std::vector<UIPanel*> getChild() = 0;


This and maybe other functions should be const.

First time I hear about const. You'd be correct, they should be.

BevapDin · 2017-05-20T10:47:32Z

src/UIHandler.h

+{
+public:
+    virtual std::vector<UIPanel*> getChild() = 0;
+    virtual void addChild(UIPanel *panel) = 0;


Here for example it's not clear who owns the panel pointer and who is responsible for deleting it.

You could write this as void addChild( std::unique_ptr<UIPanel> panel );, which clearly states the function will take care of the pointer (either by storing it or by deleting it).

Signed-off-by: Hal Gentz <[email protected]>

Leland · 2017-05-22T01:57:46Z

src/UIHandler.h

@@ -22,6 +22,7 @@ namespace Utils
 /**
 * Draws a border.
 *
+ * <pre>


Are you adding HTML elements to C++ code?

Yes, for doxygen.

Without <pre> it wraps my image into three weird lines. If you add <pre> (preformated) it doesn't wrap it.

Signed-off-by: Hal Gentz <[email protected]>

goddessfreya · 2017-05-22T04:32:13Z

I've completed reformatting the code to follow the style used almost everywhere and I think I've adressed all of the stated things. Can I get reviewed more or have this pushed?

Leland · 2017-05-23T14:20:18Z

@zegentzy can you summarize in the PR description the appreciable differences that this implements? For a good example of a PR summary, see most any of @codemime's (e.g #20003).

Reviewing the code is one thing; we also want to make sure that your intended changes are implemented accurately. So if there are visible differences, screenshots would be great, and if there are supposed to be no visible differences, definitely say so as well.

goddessfreya · 2017-05-31T02:09:16Z

So will this just rot here for the rest of eternity? Will someone tell me how to improve this?

Toothspit · 2017-05-31T03:33:01Z

51 commits, homie. Have patience, project members are thin and this is a doozie of a PR, not to mention 0.D looming. Note the dates on some other big-ish PRs =)

If it's good code today, it'll be good code tomorrow and months from now if need be.

BorkBorkGoesTheCode · 2018-01-15T07:53:04Z

@zegentzy Are you still contributing?

goddessfreya · 2018-01-15T23:27:37Z

@BorkBorkGoesTheCode I'd be willing to continue working on this project if this actually got reviewed.

Night-Pryanik · 2018-02-27T18:15:29Z

@zegentzy you can resolve merge conflicts while waiting for the review.

ZhilkinSerg · 2018-04-03T11:11:01Z

Marking as Stalled. Feel free to reopen it.

zegentzy added 3 commits May 19, 2017 17:25

sceleton

c5b1aae

Signed-off-by: Hal Gentz <[email protected]>

Still not so good

ec56fe2

Signed-off-by: Hal Gentz <[email protected]>

MOAR Progress

96501a1

Signed-off-by: Hal Gentz <[email protected]>

zegentzy added 8 commits May 20, 2017 01:41

Add size handler for UIParentPanel

46abacb

Signed-off-by: Hal Gentz <[email protected]>

MOAR Progress

698c50e

Signed-off-by: Hal Gentz <[email protected]>

I swear its fixed

79fe2de

Signed-off-by: Hal Gentz <[email protected]>

Changed gitignore

4d879c3

Signed-off-by: Hal Gentz <[email protected]>

This should work

e4702c0

Signed-off-by: Hal Gentz <[email protected]>

Screw you astyle

f7c6110

Signed-off-by: Hal Gentz <[email protected]>

Screw you astyle 2

40309ab

Signed-off-by: Hal Gentz <[email protected]>

It compiles

2791893

Scince I haven't moved anything to my panels I have zero size! High quality work here! Signed-off-by: Hal Gentz <[email protected]>

BevapDin suggested changes May 20, 2017

View reviewed changes

zegentzy added 3 commits May 20, 2017 10:49

Some fixes

8de50b3

Signed-off-by: Hal Gentz <[email protected]>

Replaced with enum

d6d6784

Signed-off-by: Hal Gentz <[email protected]>

Updated all pointers to new fancy ones

a30cade

Signed-off-by: Hal Gentz <[email protected]>

Night-Pryanik changed the title ~~[WIP] Overhualling CDDA's UI~~ [WIP] Overhaulling CDDA's UI May 20, 2017

zegentzy added 9 commits May 20, 2017 14:30

We got borders... tabs next

55fdeb1

Signed-off-by: Hal Gentz <[email protected]>

Renamed ParentPanel to PaddingPanel

770c4ac

Signed-off-by: Hal Gentz <[email protected]>

I broke something

0cbe30b

Signed-off-by: Hal Gentz <[email protected]>

Now we get a segmentation fault

7102f57

Signed-off-by: Hal Gentz <[email protected]>

works

542731c

Signed-off-by: Hal Gentz <[email protected]>

works p2

069692b

Signed-off-by: Hal Gentz <[email protected]>

Tabs complete, I'll work on controls next

9ba10f1

Signed-off-by: Hal Gentz <[email protected]>

I build with lua so I didn't catch this...

ed84298

Signed-off-by: Hal Gentz <[email protected]>

Switching tabs

5db8a96

Signed-off-by: Hal Gentz <[email protected]>

goddessfreya changed the title ~~[WIP] Overhaulling CDDA's UI~~ [WIP] Adds tab and padding panel. May 21, 2017

zegentzy added 5 commits May 21, 2017 19:46

Renamed Window.offset

294d28d

Signed-off-by: Hal Gentz <[email protected]>

Renamed Window.wf_win

485af25

Signed-off-by: Hal Gentz <[email protected]>

Renamed Window.minSize

a189654

Signed-off-by: Hal Gentz <[email protected]>

Renamed Window.thisSize

a474555

Signed-off-by: Hal Gentz <[email protected]>

Renamed Window.panel

154a61a

Signed-off-by: Hal Gentz <[email protected]>

Leland reviewed May 22, 2017

View reviewed changes

zegentzy added 8 commits May 21, 2017 20:01

Renamed everything in PaddingPanel

fbe2cbc

Signed-off-by: Hal Gentz <[email protected]>

Renamed TabPanel.drawBorder

b178750

Signed-off-by: Hal Gentz <[email protected]>

No more m_

4d88b74

Signed-off-by: Hal Gentz <[email protected]>

Enums and panel

1db9272

Signed-off-by: Hal Gentz <[email protected]>

more renaming

c8e1c3e

Signed-off-by: Hal Gentz <[email protected]>

All vars renamed

b80351c

Signed-off-by: Hal Gentz <[email protected]>

Vars renamed and undid stupid error

3565582

Signed-off-by: Hal Gentz <[email protected]>

All functions completed

b4a3225

Signed-off-by: Hal Gentz <[email protected]>

goddessfreya mentioned this pull request Jun 12, 2017

[WIP] Adds Split and Tree panel And moves more code to use them #21221

Closed

illi-kun changed the title ~~[RDY] Adds tab and padding panel.~~ Adds tab and padding panel Mar 17, 2018

ZhilkinSerg added Info / User Interface Game - player communication, menus, etc. [C++] Changes (can be) made in C++. Previously named `Code` (P2 - High) High priority (for ex. important bugfixes) (S5 - Stalled) labels Apr 2, 2018

ZhilkinSerg closed this Apr 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds tab and padding panel #21058

Adds tab and padding panel #21058

goddessfreya commented May 19, 2017 •

edited

Loading

Leland commented May 20, 2017

ZhilkinSerg commented May 20, 2017

kevingranade commented May 20, 2017

goddessfreya commented May 20, 2017

BevapDin May 20, 2017

goddessfreya May 20, 2017

BevapDin May 20, 2017

goddessfreya May 20, 2017

BevapDin May 20, 2017

goddessfreya May 20, 2017

BevapDin May 20, 2017

BevapDin May 20, 2017

BevapDin May 20, 2017

goddessfreya May 20, 2017

BevapDin May 20, 2017

goddessfreya May 20, 2017

BevapDin May 20, 2017

BevapDin May 20, 2017

goddessfreya May 20, 2017

BevapDin May 20, 2017

Leland May 22, 2017

goddessfreya May 22, 2017 •

edited

Loading

goddessfreya commented May 22, 2017

Leland commented May 23, 2017

goddessfreya commented May 31, 2017

Toothspit commented May 31, 2017 •

edited

Loading

BorkBorkGoesTheCode commented Jan 15, 2018

goddessfreya commented Jan 15, 2018

Night-Pryanik commented Feb 27, 2018

ZhilkinSerg commented Apr 3, 2018

Adds tab and padding panel #21058

Adds tab and padding panel #21058

Conversation

goddessfreya commented May 19, 2017 • edited Loading

Observable Changes

Implementation details

Leland commented May 20, 2017

ZhilkinSerg commented May 20, 2017

kevingranade commented May 20, 2017

goddessfreya commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goddessfreya May 22, 2017 • edited Loading

Choose a reason for hiding this comment

goddessfreya commented May 22, 2017

Leland commented May 23, 2017

goddessfreya commented May 31, 2017

Toothspit commented May 31, 2017 • edited Loading

BorkBorkGoesTheCode commented Jan 15, 2018

goddessfreya commented Jan 15, 2018

Night-Pryanik commented Feb 27, 2018

ZhilkinSerg commented Apr 3, 2018

goddessfreya commented May 19, 2017 •

edited

Loading

goddessfreya May 22, 2017 •

edited

Loading

Toothspit commented May 31, 2017 •

edited

Loading