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

ControlTextbox multiline overhaul, use in script editor #3036

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

Vankata453
Copy link
Member

This PR overhauls ControlTextbox so it supports multiple lines.

A new derived from ControlTextbox class has been added - ControlScriptbox. It features suggestions when typing (just like the console does when "Tab" is pressed). Additional information and parameters for the suggested functions are also displayed. That information is fetched from the "data/scripts/reference.stsr" file, which is of type "supertux-scripting-reference". Other such files can also be imported using the register_scripting_reference() and unregister_scripting_reference() scripting functions.

image

Short video showcase from 2023 development summary video

NOTE: This is a WIP, since it needs to be suited to all additions and changes since the implementation of the simplesquirrel library. It also needs to further be tested for bugs.

1. `DrawingRequest` no longer preserves a variable indicating its type, but rather utilizes a `virtual` function for this purpose.
2. `DrawingRequest` now initializes some of its variables from the current `DrawingTransform`.
2. The viewport rectangle is now stored in `DrawingTransform`, instead of `DrawingContext`. bb18239 has added per-request viewport cropping. Utilizing it now is more convenient, because the old viewport wouldn't have to be stored in a separate variable in a draw function, so it could be restored later. A new transform could be pushed and popped instead.
Includes some scripting reference generation improvements as well.
Credits to RustyBox for the new text cursor!
@Vankata453 Vankata453 added this to the 0.7.0 milestone Aug 7, 2024
@tobbi
Copy link
Member

tobbi commented Aug 7, 2024

I'll probably do some style fixes soon.

@swagtoy
Copy link
Contributor

swagtoy commented Nov 23, 2024

For reasons, Im interested in reviewing this when I get a chance :)

Copy link
Contributor

@swagtoy swagtoy left a comment

Choose a reason for hiding this comment

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

Taking a break at control_textbox.cpp line 669, but dumping what I've seen.

Just little things mostly, but overall looks good. I have not even ran this PR yet.

ItemScriptField::get_height() const
{
return static_cast<int>(m_control->get_rect().get_height());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall these (get_width/get_height) go to the header?

bool locks_navigation() const override { return true; }
bool locked() const override { return m_control->suggestions_active(); }
bool select_blink() const override { return false; }
bool overrides_cursor_state() const override { return true; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh?

I suppose this is a practice around here; why aren't we using member variables for this? Smells Java-esque.

virtual bool locks_navigation() const { return false; }

/** Returns true when the menu's focus is locked to this item. */
virtual bool locked() const { return false; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I don't know what to think about this. Follow up to me if you can

m_function_parameters(),
m_hovered_parameter(nullptr)
{
m_suggestions_scrollbar.reset(new ControlScrollbar(1.f, SUGGESTION_RECT_HEIGHT, m_suggestions_offset, 35.f));
Copy link
Contributor

Choose a reason for hiding this comment

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

std::make_unique

{
ControlTextbox::draw(context);

/** Line numbers */
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a very helpful comment, also /** is javadoc starter i think. I don't typically see this.

Could also go into a private function anyway. Maybe this logic could be refactored a bit if you want to do that?


return false;
}

bool
ControlTextbox::parse_value(bool call_on_change /* = true (see header)*/)
ControlTextbox::parse_value(bool call_on_change /* = true (see header)*/, TextAction action /* = TextAction::NONE (see header)*/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just don't (see header) the reader like this at all. They'll see the header if they need to, as it's obvious.

if (m_string)
*m_string = new_str;
if (m_string)
*m_string = new_str;
Copy link
Contributor

Choose a reason for hiding this comment

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

what if instead of a pointer we just set some int variables or something, then returned a string view of some sort? Food for thought of the soul. It is certainly how most editors handle such logic, if I'm getting this down correctly

Comment on lines +549 to 562
int line = 0;
for (char c : str)
{
if (c == '\n')
{
if (!m_multiline)
continue;

m_charlist.push_back({});
line++;
continue;
}
m_charlist[line].push_back(c);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Standard for statement can't possibly hurt in this case. Not a big deal though.

@@ -361,102 +574,125 @@ ControlTextbox::get_string() const
}

std::string
ControlTextbox::get_contents() const
ControlTextbox::get_contents(int line) const
Copy link
Contributor

Choose a reason for hiding this comment

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

This does handle negative logic too? I think it can be made clear with a comment.

Comment on lines -376 to 613
ControlTextbox::get_first_chars(int amount) const
ControlTextbox::get_first_chars(int line, int amount) const
{
std::string temp;
std::string out;

for (char c : m_charlist) {
for (char c : m_charlist[line])
{
if (!(amount--)) break;
temp += c;
out += c;
}

return temp;
return out;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this logic be improved somehow, anyway?

@Alasdairbugs Alasdairbugs modified the milestones: 0.7.0, 0.7.1 Feb 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants