Formspec refactor proposal

[Unarelith]

I started a refactor on GUIFormSpecMenu and I replaced BoxDrawSpec by a new class BoxWidget.

The goal of this PR is to split all the widget-specific code from the main class.

This PR is a proposal. I know this kind of changes probably won't be merged before 5.0.0, but I won't add new stuff until your review anyway.

NB: m_box_widgets is temporary since Widget will be used as an interface for all widgets.

[stujones11]

@Quent42340 I am mostly in favour of your refactors but these should definitely be done after 5.0.0 gets released.

IMO, GUIFormSpecMenu needs to be re-written, not just refactored :)

[Unarelith]

@Quent42340 I am mostly in favour of your refactors but these should definitely be done after 5.0.0 gets released.

@stujones11 the main goal is to discuss this change here until 5.0.0 is released. That's why this change is incomplete.

IMO, GUIFormSpecMenu needs to be re-written, not just refactored :)

Well, I don't totally agree with you since refactoring this class will help if a rewrite is ever made.

The proposal in this PR consists in a adding a more generic way to store/draw/parse widget data. If a rewrite is ever made, existing code could still be reused for some parts (for example widget code), hence it will be easier to reuse refactored code than to refactor useful parts while rewriting.

[stujones11]

this class will help if a rewrite is ever made.

Sorry, I really meant formspecs should be replaced altogether but the refactoring is fine since that is not likely to happen any time soon. I was mostly trying to stress the importance of doing this after a release as I have spent a lot of time getting this working properly on android. I am sure any breakages would be easily fixed, I'd just like to see 5.0.0 go out this century :)

[Unarelith]

Sorry, I really meant formspecs should be replaced altogether but the refactoring is fine since that is not likely to happen any time soon.

Probably for 6.0.0 since this is a breaking change (and a big one). But there's two different things to consider here:

• Rendering of formspecs
• formspecs themselves and Lua API around them

If a rewrite is ever made, it will have to deal with both. However, one could easily write a new and alternative rendering system if the abstraction layer is good enough, while keeping current formspecs., so that would split the work on that part.

I'd just like to see 5.0.0 go out this century :)

I can't agree more, that's why this PR is incomplete and was only open for discussion. I'll finish it when 5.0.0 will be released.

[rubenwardy]

Probably for 6.0.0 since this is a breaking change (and a big one)

I don't see why this needs to be a breaking change

[Unarelith]

Well, considering that the goal would be to replace formspecs completely, it is a breaking change.

[rubenwardy]

Formspecs shouldn't be replaced completely, this would break the majority of mods

[Unarelith]

Actually they should, and the community will help upgrading existing mods if the new GUI system is first developed as an alternative before formspecs get deprecated.

[rubenwardy]

Formspecs should be deprecated, but not removed, is my point

[Unarelith]

I agree that they should be deprecated first, but they will eventually be removed later, once everybody uses the new GUI system.

I'm currently able to work on this, but I won't spend time making something that'll never be merged.

So please, tell me if this PR is worth or not, and tell me when I can work on it, and by that I mean when someone's able to review it so that I don't rebase it a lot of times.

[rubenwardy]

I'm worried about how to ensure that this doesn't break compatibility / cause issues. I'm also wondering if this is even worth it if we are to replace formspecs soon:tm: anyway

[SmallJoker]

Also I'd like to see #8524 merged before this PR. Correct coordinates to make it less awful to use have a higher priority than refactoring. Still, both PRs will arise rebase conflicts in the other PR.

[Unarelith]

@rubenwardy Well since you don't want to remove formspecs completely I think a refactoring won't hurt
Is there a PR for a new GUI system? Is it planned for this year? If not, seems worth.
And I can split this into multiple meaningful commits to make reviewing and testing easier.

@SmallJoker Don't forget that this PR is not finished at all, it was only the beginning of a bigger refactoring.
I can restart it from scratch when #8524 will be merged, won't take much time.

[rubenwardy]

I've decided that this would actually be quite a good idea, as we're unlikely to get a full formspec replacement soon and this at least makes it easier to maintain in the meantime

[Unarelith]

I'll need you to tell me when I can work on this.

There's a lot of formspec-related PRs waiting for review, and I think these should be prioritized since it'll be harder to rebase them after this kind of refactoring.

[SmallJoker]

@Unarelith https://dev.minetest.net/Meetings#2019-06-X
EDIT: Yours does not seem to be on the list yet. Maybe after style[] since it's the last large PR in terms of modified Minetest code.

[v-rob]

Actually, I think scroll_container does some big modifications on how certain things are drawn, and merging this first would likely break all that work. Since that one's already done and this one isn't, I think that this should be merged afterwords.

[rubenwardy]

Out of date and inactive

[Unarelith]

This PR was an example of how I could rework the formspec code, I'm still around and I could start making more work on it starting from next week, unless something as already been done about that and this PR isn't relevant anymore?

[Unarelith]

Ok so after rebasing #7971 into #9240, I noticed that this PR could still be useful.

So @rubenwardy, @paramat, just to remind both of you why I did this PR in the first place:

When someone want to make changes to formspec code, that person would have to edit guiFormSpecMenu.h and guiFormSpecMenu.cpp, because everything is in these two files.
The problem is that if several people work together on these files, there's a lot of conflicts to handle and, imo, it's way harder to review.

The goal of this PR was to split GUIFormSpecMenu class into smaller classes handling one smaller thing at a time, helping everybody to work on it.

But the issue with this PR was the amount of other PR related to formspecs, waiting to be merged. So mine would have prevent these PRs to be merged, and thus I had to wait for them because doing it. And that's also why I didn't finished it, it's was more like a POC.

I don't know the current state of formspec PRs, but if I can work on this one, I'll do it. However, if I work on this one, it's to get it merged, not to wait for a year or something to rebase it, becase with that kind of PR it's impossible and I'll have to start over.

So I just need you guys to tell me what I can do on this one.

[paramat]

As far as i know there is a lot of activity in formspec stuff at the moment, so this may have to wait a while. However, i am mostly clueless when it comes to formspecs and the state of formspec stuff, as it is an area of code i am not good with and intentionally ignore. Rubenwardy probably has the best grasp of what is going on.
(Runs away.)
=)

[Unarelith]

Well, there is this project: https://github.com/minetest/minetest/projects/6
And what I just said is so much relevant if you read the name:

List of PRs which conflict in their formspec code changes. Please check this list before changing too much in guiFormspecMenu!

Every formspec PR is going to create conflicts if everything stays in the same file. I think it's easy to realize how much this PR can ease both maintainers and contributors life.

Anyway, I'm not going to wait forever, most of the waiting PRs are quite small, WIP or paused for quite some time. What I mean is that these PRs are easy to recreate like I did for #9240, and if they're not then they should be merged first, but there's not a lot of big ones.

If you don't decide at some point to do a refactoring, the code will end up being just a pile of patches with files over 3000+ lines and it won't be maintainable anymore. And actually, it's already like this.

[v-rob]

I think that splitting into multiple files would be a good idea to do at any time. Sure, it will make it so that every PR has to be rebased, but hey, that's already happening every time one is merged. Plus, it will make it so that there will be rebases less frequently overall, I think.

Really, some of the functions in guiFormSpecMenu.cpp are hundreds of lines long, which is ridiculous. They need to be broken down, and it will make life for everyone better.

[SmallJoker]

http://irc.minetest.net/minetest-dev/2019-12-30#i_5627756

FormSpecData is a good idea to bundle the render information, but passing it to each parse function is not elegant at all
if even, bundle more into that struct (or class) so that everything that's needed for adding an element is together in one argument