fix for scrollbar segfaults #251

2020-02-06T02:04:17Z

x-qq commented

2020-02-06 02:04:17 +00:00

(Migrated from github.com)

fixes Clicking on scrollbar crashes/segfaults (linux) (#206)
fixes broken mousewheel scrolling of the scenedit palette
removed boost threads dependency
added foundation for further refactoring of the drawing and event handling code: interfaces and drawable manager with layering
removed a bunch of unneeded redraw calls
removed some repeated recalculation of effectively constant values (boe.actions)
removed recalculation of effectively constant scrollbar and button positions (boe.graphics)

* fixes #206 * fixes broken mousewheel scrolling of the scenedit palette * removed boost threads dependency * added foundation for further refactoring of the drawing and event handling code: interfaces and drawable manager with layering * removed a bunch of unneeded redraw calls * removed some repeated recalculation of effectively constant values (boe.actions) * removed recalculation of effectively constant scrollbar and button positions (boe.graphics)

CelticMinstrel (Migrated from github.com) reviewed 2020-02-09 00:48:39 +00:00

CelticMinstrel (Migrated from github.com) left a comment

On the whole this looks fine. I'll probably merge it in for testing purposes "soon" and see how well it runs. If I notice anything wrong I'll mention it then. Until then, here's all my comments from just reading through the diff.

General style points, not high priority as I can change them myself after merge, but if you want to take the time to change them yourself, feel free:

No space after if, while, switch, for, function names, etc
No space before the opening < in template parameters (both in defining the template and in instantiating it), nor after the closing > (when calling a template function or using a C++ cast keyword)
const someType& instead of someType const &
In classes, public:, protected:, and private: should be indented to the same level as the class header, ie one level less than you're doing.

On the whole this looks fine. I'll probably merge it in for testing purposes "soon" and see how well it runs. If I notice anything wrong I'll mention it then. Until then, here's all my comments from just reading through the diff. General style points, not high priority as I can change them myself after merge, but if you want to take the time to change them yourself, feel free: * No space after `if`, `while`, `switch`, `for`, function names, etc * No space before the opening `<` in template parameters (both in defining the template and in instantiating it), nor after the closing `>` (when calling a template function or using a C++ cast keyword) * `const someType&` instead of `someType const &` * In classes, `public:`, `protected:`, and `private:` should be indented to the same level as the class header, ie one level less than you're doing.

src/dialogxml/widgets/scrollbar.cpp

						
				@@ -62,6 +62,10 @@ void cScrollbar::setStyle(eScrollStyle newStyle) {

					style = newStyle;

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:12:25 +00:00

This name is too long... can we call it something like set_controlled_rect or set_pane_rect (after the cScrollPane class)?

This name is too long... can we call it something like `set_controlled_rect` or `set_pane_rect` (after the cScrollPane class)?

src/dialogxml/widgets/scrollbar.cpp

						
				@@ -86,6 +90,190 @@ eScrollStyle cScrollbar::getStyle() {

					return style;

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:16:49 +00:00

I think this should check both the "catching" / "controlled" rect and the rect of the scrollbar itself. Obviously the former rect can generally be arranged to encompass the latter rect, but in case for some reason it isn't (maybe it's being used as a slider instead of a scrollbar), using the wheel while hovering the bar should probably still work.

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:18:54 +00:00

This isn't an issue per se but I kinda prefer trailing return types here as it eliminates the need to qualify the nested type:

auto cScrollbar::location_to_part(...) const -> eScrollbarPart

(I think this type should basically never be qualified anyway, unless used from outside the class. Well, if it was called ePart instead of eScrollbarPart, I might not mind it always being qualified...)

This isn't an issue per se but I kinda prefer trailing return types here as it eliminates the need to qualify the nested type: `auto cScrollbar::location_to_part(...) const -> eScrollbarPart` (I think this type should basically never be qualified anyway, unless used from outside the class. Well, if it was called ePart instead of eScrollbarPart, I might not mind it always being qualified...)

src/dialogxml/widgets/scrollbar.hpp

						
				@@ -45,7 +47,20 @@ class cScrollbar : public cControl {

					bool vert = true;

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:22:19 +00:00

As with the function to set it, this name is too long. (I think explicitly initializing it also isn't necessary, but it can't hurt either, I guess.)

Also as I mentioned elsewhere, there's not really any need to force it to cover the scrollbar itself.

As with the function to set it, this name is too long. (I think explicitly initializing it also isn't necessary, but it can't hurt either, I guess.) Also as I mentioned elsewhere, there's not really any need to force it to cover the scrollbar itself.

src/game/boe.actions.cpp

						
				@@ -2114,29 +2113,19 @@ bool handle_keystroke(const sf::Event& event){

				}

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:23:47 +00:00

Can you please write TODO comments as // TODO: ...? Note the colon. (C-style TODO comments are fine too, for the record.)

Can you please write TODO comments as `// TODO: ...`? Note the colon. (C-style TODO comments are fine too, for the record.)

src/game/boe.actions.cpp

						
				@@ -2123,3 +2121,4 @@

					location pos(event.mouseWheel.x, event.mouseWheel.y);

					pos = mainPtr.mapPixelToCoords(pos, mainView);

					int amount = event.mouseWheel.delta;

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:24:50 +00:00

It's not immediately clear why these were removed, but... I guess they're now set as the control rect of the two scrollbars?

src/game/boe.consts.hpp

						
				@@ -164,2 +162,4 @@

				const int UI_LAYER_DEFAULT = 1000;

				const int UI_LAYER_MENUBAR = 1200;

				#endif

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:25:52 +00:00

Why were the other two constants removed here?

src/game/boe.main.cpp

						
				@@ -2,9 +2,10 @@

				#include "boe.global.hpp"

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:27:44 +00:00

This relates to my Mac (Xcode 4) build, but given that it specifically references Boost.Thread which we're no longer using, it's probably safe to remove it.

src/game/boe.main.cpp

						
				@@ -41,9 +45,10 @@ bool first_sound_played = false,spell_forced = false;

				bool party_in_memory = false;

				std::shared_ptr<cScrollbar> text_sbar, item_sbar, shop_sbar;

				std::shared_ptr<cButton> done_btn, help_btn;

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:32:46 +00:00

Please use either TODO: or FIXME: for todo-comments, instead of XXX.

Please use either `TODO:` or `FIXME:` for todo-comments, instead of `XXX`.

src/game/boe.main.cpp

						
				@@ -91,6 +99,10 @@ effect_pat_type current_pat;

				short missile_firer,current_monst_tactic;

				short store_current_pc = 0;

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:33:22 +00:00

Again, TODO: or FIXME: prefix instead of a XXX suffix.

Again, `TODO:` or `FIXME:` prefix instead of a `XXX` suffix.

src/game/boe.main.cpp

						
				@@ -135,21 +147,65 @@ int main(int argc, char* argv[]) {

					}

				}

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:35:25 +00:00

I realize this is a silly little thing, but can we perhaps call this "transcript-scrollbar"?

I realize this is a silly little thing, but can we perhaps call this `"transcript-scrollbar"`?

src/game/boe.main.cpp

						
				@@ -167,3 +220,1 @@

					init_sbar(shop_sbar, shop_sbar_rect, 16, 8);

					init_btn(done_btn, BTN_DONE);

					init_btn(help_btn, BTN_HELP);

					init_ui();

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:34:06 +00:00

Oh, "events_rect" is nice, maybe that can be used as its name within the cScrollbar class. Or something more similar to it.

Oh, "events_rect" is nice, maybe _that_ can be used as its name within the cScrollbar class. Or something more similar to it.

src/game/boe.main.cpp

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:36:05 +00:00

In fact, at some point probably the entire init_ui could be moved there...

In fact, at some point probably the entire `init_ui` could be moved there...

src/game/boe.main.cpp

						
				@@ -264,6 +317,7 @@ void handle_one_event(const sf::Event& event) {

							change_cursor({event.mouseMove.x, event.mouseMove.y});

							return;

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:37:20 +00:00

Another improperly-formatted TODO note.

I'm probably building with a rather older SFML, too, so this is far from a priority.

Another improperly-formatted TODO note. I'm probably building with a rather older SFML, too, so this is far from a priority.

src/game/boe.menu.hpp

						
				@@ -21,2 +21,4 @@

					virtual bool handle_event(const sf::Event&) override;

					virtual void draw() override;

					void update_for_game_state(eGameMode overall_mode, bool party_in_memory);

					void update_spell_menus();

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:38:56 +00:00

You don't actually need the virtual keyword here as the virtual property is inherited... not that it hurts or anything.

You don't actually _need_ the virtual keyword here as the virtual property is inherited... not that it hurts or anything.

src/scenedit/scen.global.hpp

						
				@@ -19,6 +19,9 @@ const int RIGHT_AREA_HEIGHT = 400;

				const int TER_RECT_UL_X = 6;

				const int TER_RECT_UL_Y = 19;

				const int UI_LAYER_DEFAULT = 1000;

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:40:53 +00:00

Seeing this constants duplicated in two places makes me wonder if they should be in the interface header instead...

src/scenedit/scen.main.cpp

						
				@@ -66,6 +67,10 @@ extern rectangle terrain_buttons_rect;

				extern void set_up_apple_events(int argc, char* argv[]);

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:41:27 +00:00

More wrongly-formatted TODO comments.

src/scenedit/scen.main.cpp

CelticMinstrel (Migrated from github.com) commented

2020-02-09 00:43:06 +00:00

Feels like there's no reason for the Mouse_Pressed function to even exist anymore...

Feels like there's no reason for the `Mouse_Pressed` function to even exist anymore...

x-qq (Migrated from github.com) reviewed 2020-02-09 01:44:40 +00:00

src/game/boe.actions.cpp

						
				@@ -2123,3 +2121,4 @@

					location pos(event.mouseWheel.x, event.mouseWheel.y);

					pos = mainPtr.mapPixelToCoords(pos, mainView);

					int amount = event.mouseWheel.delta;

x-qq (Migrated from github.com) commented

2020-02-09 01:44:40 +00:00

Yes. And these magic numbers were not even used as-is, they were "adjusted" using .offset before being used.

And this calculation was performed per every mousewheel event processed.

So I removed this and instead use the "already adjusted" values as event catching rects.

Yes. And these magic numbers were not even used as-is, they were "adjusted" using .offset before being used. And this calculation was performed per every mousewheel event processed. So I removed this and instead use the "already adjusted" values as event catching rects.

x-qq (Migrated from github.com) reviewed 2020-02-09 01:46:36 +00:00

src/game/boe.consts.hpp

						
				@@ -164,2 +162,4 @@

				const int UI_LAYER_DEFAULT = 1000;

				const int UI_LAYER_MENUBAR = 1200;

				#endif

x-qq (Migrated from github.com) commented

2020-02-09 01:46:36 +00:00

They were introduced during linux menubar implementation, but never actually used. I forgot to remove them then.

x-qq (Migrated from github.com) reviewed 2020-02-09 01:52:24 +00:00

src/game/boe.main.cpp

						
				@@ -135,21 +147,65 @@ int main(int argc, char* argv[]) {

					}

				}

x-qq (Migrated from github.com) commented

2020-02-09 01:52:24 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:52:40 +00:00

src/game/boe.main.cpp

						
				@@ -91,6 +99,10 @@ effect_pat_type current_pat;

				short missile_firer,current_monst_tactic;

				short store_current_pc = 0;

x-qq (Migrated from github.com) commented

2020-02-09 01:52:40 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:52:48 +00:00

src/game/boe.main.cpp

						
				@@ -41,9 +45,10 @@ bool first_sound_played = false,spell_forced = false;

				bool party_in_memory = false;

				std::shared_ptr<cScrollbar> text_sbar, item_sbar, shop_sbar;

				std::shared_ptr<cButton> done_btn, help_btn;

x-qq (Migrated from github.com) commented

2020-02-09 01:52:48 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:52:56 +00:00

src/game/boe.main.cpp

						
				@@ -2,9 +2,10 @@

				#include "boe.global.hpp"

x-qq (Migrated from github.com) commented

2020-02-09 01:52:55 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:53:21 +00:00

src/dialogxml/widgets/scrollbar.hpp

						
				@@ -45,7 +47,20 @@ class cScrollbar : public cControl {

					bool vert = true;

x-qq (Migrated from github.com) commented

2020-02-09 01:53:21 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:53:34 +00:00

src/dialogxml/widgets/scrollbar.cpp

						
				@@ -86,6 +90,190 @@ eScrollStyle cScrollbar::getStyle() {

					return style;

x-qq (Migrated from github.com) commented

2020-02-09 01:53:34 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:53:42 +00:00

src/dialogxml/widgets/scrollbar.cpp

						
				@@ -86,6 +90,190 @@ eScrollStyle cScrollbar::getStyle() {

					return style;

x-qq (Migrated from github.com) commented

2020-02-09 01:53:41 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:53:49 +00:00

src/dialogxml/widgets/scrollbar.cpp

						
				@@ -62,6 +62,10 @@ void cScrollbar::setStyle(eScrollStyle newStyle) {

					style = newStyle;

x-qq (Migrated from github.com) commented

2020-02-09 01:53:49 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:54:08 +00:00

src/game/boe.main.cpp

						
				@@ -264,6 +317,7 @@ void handle_one_event(const sf::Event& event) {

							change_cursor({event.mouseMove.x, event.mouseMove.y});

							return;

x-qq (Migrated from github.com) commented

2020-02-09 01:54:08 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:56:09 +00:00

src/scenedit/scen.global.hpp

						
				@@ -19,6 +19,9 @@ const int RIGHT_AREA_HEIGHT = 400;

				const int TER_RECT_UL_X = 6;

				const int TER_RECT_UL_Y = 19;

				const int UI_LAYER_DEFAULT = 1000;

x-qq (Migrated from github.com) commented

2020-02-09 01:56:09 +00:00

Layer ids are per application values. And since game and editors are different applications I think this is fine.

x-qq (Migrated from github.com) reviewed 2020-02-09 01:56:17 +00:00

src/scenedit/scen.main.cpp

						
				@@ -66,6 +67,10 @@ extern rectangle terrain_buttons_rect;

				extern void set_up_apple_events(int argc, char* argv[]);

x-qq (Migrated from github.com) commented

2020-02-09 01:56:17 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:57:32 +00:00

src/game/boe.actions.cpp

						
				@@ -2114,29 +2113,19 @@ bool handle_keystroke(const sf::Event& event){

				}

x-qq (Migrated from github.com) commented

2020-02-09 01:57:31 +00:00

done

x-qq (Migrated from github.com) reviewed 2020-02-09 01:58:05 +00:00

src/game/boe.main.cpp

x-qq (Migrated from github.com) commented

2020-02-09 01:58:05 +00:00

Added note.

x-qq (Migrated from github.com) reviewed 2020-02-09 01:59:02 +00:00

src/game/boe.menu.hpp

						
				@@ -21,2 +21,4 @@

					virtual bool handle_event(const sf::Event&) override;

					virtual void draw() override;

					void update_for_game_state(eGameMode overall_mode, bool party_in_memory);

					void update_spell_menus();

x-qq (Migrated from github.com) commented

2020-02-09 01:59:02 +00:00

I will leave this as is.

x-qq (Migrated from github.com) reviewed 2020-02-09 01:59:53 +00:00

src/scenedit/scen.main.cpp

x-qq (Migrated from github.com) commented

2020-02-09 01:59:53 +00:00

Coordinate translation is a long statement, I do not like having them inline, so I'll leave this as is.

x-qq commented

2020-02-09 02:09:09 +00:00

(Migrated from github.com)

I have adjusted some whitespace as requested.

Please note however that I do not intend, under any circumstances, to use "const before" notation.

I have adjusted some whitespace as requested. Please note however that I do not intend, under any circumstances, to use "const before" notation.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: openboe/oboe#251

				`@@ -62,6 +62,10 @@ void cScrollbar::setStyle(eScrollStyle newStyle) {`
				`style = newStyle;`

				`@@ -86,6 +90,190 @@ eScrollStyle cScrollbar::getStyle() {`
				`return style;`

				`@@ -45,7 +47,20 @@ class cScrollbar : public cControl {`
				`bool vert = true;`

				`@@ -2114,29 +2113,19 @@ bool handle_keystroke(const sf::Event& event){`
				`}`

				`@@ -66,6 +67,10 @@ extern rectangle terrain_buttons_rect;`

				`extern void set_up_apple_events(int argc, char* argv[]);`

				`@@ -2,9 +2,10 @@`
				`#include "boe.global.hpp"`

				`@@ -2,9 +2,10 @@`
				`#include "boe.global.hpp"`