Freeciv

Reply To cazfi

Ruledit should have tab for counters editing. Initial version can have as little functionality as needed to make it to provide any value at all (likely it's easy to get functionality common to most ruleset objects by simply copying, e.g., extras tab and modify that) Any counters removal functionality (and basically adding too, as that would be unreversable action without ability to remove the counter) depends on #45041

Initial patch only.

Reply To cazfi

Ruledit should have tab for counters editing. Initial version can have as little functionality as needed to make it to provide any value at all (likely it's easy to get functionality common to most ruleset objects by simply copying, e.g., extras tab and modify that) Any counters removal functionality (and basically adding too, as that would be unreversable action without ability to remove the counter) depends on #45041

I cannot find in UI, how to add new entitiy or remove selected. I will look inside code.

@cazfi: 0001-OSDN-TICKET-45892-S-awomir-Lach-slawek-lach.art.pl.patch(11KB)

I see there is bug with saving. Probably - saving part was introduced before other counter behavior than OWNED is introduced, so it not saving any changes. It is always OWNED behavior set, for example.

To see entire feature-set, just compile with --enable-ruledit=experimental

@cazfi:

I look inside rulesave and discovered code is good. I discovered it was my mistake. I prefer gtk+ than QT and do spelling bug in signal handler registration (missing '()'), so signal would not be called. Now, everything seems to work. I also added changing checkpoint and default value. I also integrate this patch with previous, so necessary changes into Makefile.am/Makefile.in would be applied.

In general the code looks promising already.

- Meson build is broken; update meson.build about the new files
- QRegExp is deprecated, and available to Qt6-client only via Qt5compat: https://doc.qt.io/qt-6/qregexp.html
- As there are new translatable strings in new files, update translations/ruledit/POTFILES.in
- Only "Add" and "Delete" would need to be limited to experimental builds only, I think. We can have safely have editing of the other values in normal build
- You should iterate over all counter types instead of making the assumption that there's just two specific types (assumption that would break as soon as #45489 gets implemented)
- Many unnecessary empty lines where they should not be
- Some "selected" comparison against 0 which should be nullptr
- Do not use atoi(), but something more secure, e.g., str_to_int() or str_to_uint()
- Is atoi() the reason you include cstdlib? If there's (also) some other reason, it likely needs some change (either use of some utility/ function, or C++, or Qt)
- tab_counters.h seems to have several forward declarations of Qt classes that it doesn't actually use. Didn't check if .cpp file has similar excess includes
- "#endif // FC__TAB_TECH_H" - should be ...COUNTER...
- You should check is_counter_needed() before deleting it

Reply To cazfi

In general the code looks promising already. - Meson build is broken; update meson.build about the new files
- QRegExp is deprecated, and available to Qt6-client only via Qt5compat: https://doc.qt.io/qt-6/qregexp.html
- As there are new translatable strings in new files, update translations/ruledit/POTFILES.in
- Only "Add" and "Delete" would need to be limited to experimental builds only, I think. We can have safely have editing of the other values in normal build
- You should iterate over all counter types instead of making the assumption that there's just two specific types (assumption that would break as soon as #45489 gets implemented)
- Many unnecessary empty lines where they should not be
- Some "selected" comparison against 0 which should be nullptr
- Do not use atoi(), but something more secure, e.g., str_to_int() or str_to_uint()
- Is atoi() the reason you include cstdlib? If there's (also) some other reason, it likely needs some change (either use of some utility/ function, or C++, or Qt)
- tab_counters.h seems to have several forward declarations of Qt classes that it doesn't actually use. Didn't check if .cpp file has similar excess includes
- "#endif // FC__TAB_TECH_H" - should be ...COUNTER...
- You should check is_counter_needed() before deleting it

-TICKET-45892-S-awomir-Lach-slawek-lach.art.pl.patch(18KB)

I am not sure if it would compile for Qt6. I must install another container and install only Qt6 in it. But at worst... When I install Qt6 and uninstall Qt6 on OpenSUSE Tumbleweed container, it complains about Qt version is too old. I test and it requires Qt 6.0.0, but I had 6.2 on my system. I am sure at near 100% that code formatting is good from now.

Previous patch delivers Qt6 version, which even do not build. This patch works. Also, two changes were made: 1. selected = 0 -> selected = nullptr (in constructor)' 2. Def and Checkpoint edit widgets will be fill witch data, when data update is performing

Reply To cazfi

In general the code looks promising already. - Meson build is broken; update meson.build about the new files
- QRegExp is deprecated, and available to Qt6-client only via Qt5compat: https://doc.qt.io/qt-6/qregexp.html
- As there are new translatable strings in new files, update translations/ruledit/POTFILES.in
- Only "Add" and "Delete" would need to be limited to experimental builds only, I think. We can have safely have editing of the other values in normal build
- You should iterate over all counter types instead of making the assumption that there's just two specific types (assumption that would break as soon as #45489 gets implemented)
- Many unnecessary empty lines where they should not be
- Some "selected" comparison against 0 which should be nullptr
- Do not use atoi(), but something more secure, e.g., str_to_int() or str_to_uint()
- Is atoi() the reason you include cstdlib? If there's (also) some other reason, it likely needs some change (either use of some utility/ function, or C++, or Qt)
- tab_counters.h seems to have several forward declarations of Qt classes that it doesn't actually use. Didn't check if .cpp file has similar excess includes
- "#endif // FC__TAB_TECH_H" - should be ...COUNTER...
- You should check is_counter_needed() before deleting it

Read above post. There is also need for add Counter to Requirement Edit Dialog. I will open a new ticket for this work.

Reply To lachu

Reply To cazfi

- QRegExp is deprecated, and available to Qt6-client only via Qt5compat: https://doc.qt.io/qt-6/qregexp.html

1) There's no need to have Qt5 and Qt6 implementation to differ here by using QRegExp on Qt5, as the replacements exist in Qt-5.11 already.
2) This is used to validate that a string is actually a number. You should use widget that only accepts numbers in the first place. Likely QSpinBox

- Only "Add" and "Delete" would need to be limited to experimental builds only, I think. We can have safely have editing of the other values in normal build

- Put them to the last row, not in the middle.
- While adjusting that, use a 'row' counter to place widgets, instead of maintaining plain row numbers in the source code (forcing developer to update all of them when there is any change)

- You should iterate over all counter types instead of making the assumption that there's just two specific types (assumption that would break as soon as #45489 gets implemented)

Still not done

- Many unnecessary empty lines where they should not be

- Still there
- There's also empty lines missing where they should be (between variable declarations and the code)

- Do not use atoi(), but something more secure, e.g., str_to_int() or str_to_uint()

Improved a bit by using strtol(), but still not str_to_int()

- You should check is_counter_needed() before deleting it

This should use 'requirers_dlg' to give proper feedback to the user. See how the same thing is implemented in the other tabs

+ }
+ else {

- Should be on one line
- The toggle to make "name" same as "rule name" is not there
- tab_counter::counter_name() is not needed
- There's no effect editing button. Slot edit_effects() exist, but is never signaled
- I'd prefer that "Counters" tab would be before the "Nations" tab (that the "Nations" tab with not much functionality would be the last)

Some issues were addressed by this patch, but not all.

- Use QSpinBox instead of QEditLine for number filed - Add one empty line between variable declarations and code - Remove spaces in lines with no visible text - Reorganized counters tab - Use column and row variables (and add macrodefinitions to place elements) - Move counters before nations

Reply To cazfi

Reply To lachu

Reply To cazfi

- QRegExp is deprecated, and available to Qt6-client only via Qt5compat: https://doc.qt.io/qt-6/qregexp.html

1) There's no need to have Qt5 and Qt6 implementation to differ here by using QRegExp on Qt5, as the replacements exist in Qt-5.11 already.
2) This is used to validate that a string is actually a number. You should use widget that only accepts numbers in the first place. Likely QSpinBox

- Only "Add" and "Delete" would need to be limited to experimental builds only, I think. We can have safely have editing of the other values in normal build

- Put them to the last row, not in the middle.
- While adjusting that, use a 'row' counter to place widgets, instead of maintaining plain row numbers in the source code (forcing developer to update all of them when there is any change)

- You should iterate over all counter types instead of making the assumption that there's just two specific types (assumption that would break as soon as #45489 gets implemented)

Still not done

- Many unnecessary empty lines where they should not be

- Still there
- There's also empty lines missing where they should be (between variable declarations and the code)

- Do not use atoi(), but something more secure, e.g., str_to_int() or str_to_uint()

Improved a bit by using strtol(), but still not str_to_int()

- You should check is_counter_needed() before deleting it

This should use 'requirers_dlg' to give proper feedback to the user. See how the same thing is implemented in the other tabs
+ }
+ else {
- Should be on one line
- The toggle to make "name" same as "rule name" is not there
- tab_counter::counter_name() is not needed
- There's no effect editing button. Slot edit_effects() exist, but is never signaled
- I'd prefer that "Counters" tab would be before the "Nations" tab (that the "Nations" tab with not much functionality would be the last)

Fast fix. I missed int in signal/slot reference, so it do not update value. I do not test this, because I stood my changes do not touch this functionality.

Reply To cazfi

Reply To lachu

Reply To cazfi

- QRegExp is deprecated, and available to Qt6-client only via Qt5compat: https://doc.qt.io/qt-6/qregexp.html

1) There's no need to have Qt5 and Qt6 implementation to differ here by using QRegExp on Qt5, as the replacements exist in Qt-5.11 already.
2) This is used to validate that a string is actually a number. You should use widget that only accepts numbers in the first place. Likely QSpinBox

- Only "Add" and "Delete" would need to be limited to experimental builds only, I think. We can have safely have editing of the other values in normal build

- Put them to the last row, not in the middle.
- While adjusting that, use a 'row' counter to place widgets, instead of maintaining plain row numbers in the source code (forcing developer to update all of them when there is any change)

- You should iterate over all counter types instead of making the assumption that there's just two specific types (assumption that would break as soon as #45489 gets implemented)

Still not done

- Many unnecessary empty lines where they should not be

- Still there
- There's also empty lines missing where they should be (between variable declarations and the code)

- Do not use atoi(), but something more secure, e.g., str_to_int() or str_to_uint()

Improved a bit by using strtol(), but still not str_to_int()

- You should check is_counter_needed() before deleting it

This should use 'requirers_dlg' to give proper feedback to the user. See how the same thing is implemented in the other tabs
+ }
+ else {
- Should be on one line
- The toggle to make "name" same as "rule name" is not there
- tab_counter::counter_name() is not needed
- There's no effect editing button. Slot edit_effects() exist, but is never signaled
- I'd prefer that "Counters" tab would be before the "Nations" tab (that the "Nations" tab with not much functionality would be the last)

1. I set maximum number and minimum number for QSpinBox widgets, but do not known, which values are correct

2. There is error - it saves also counters made ruledit_disabled (I tested for counters residing in the middle before deletion) - I known deleting other entities works, so I must look at the code

3. Requirement editor do not restrict range to city, but is this for another patch (I suppose, yes)?

Sorry for previous patch, I removed already. I apply ruleset patch to test and before adding files to staging area, I do git reset --hard HEAD~1 to remove changes in ruleset and apply my changes with commit --amend . In result, I send patch without changes, but now I restore my changes.

The requirement dialog problem is: I must restrict range to city only for Checkpoint.

I also lost int in connect invocation. But this patch is correct, I think:

I discovered, that there is no restriction about, which range ruleset's author set into requirement. No matter of other values of requirement, so this is no problem with my code.

Reply To cazfi

Hi. As far as I can see, it is done. I forgot to attach to your post, so you probably did not receive notification.

- slots for checkpoint and default value do not check if there's a counter selected. Program crashes if one just goes to Counters tab and touches those widgets without selecting a counter
- Coding Style of add_widget() and add_widget_next() macros is really bad (I don't particularly like them in general, but they are acceptable)
- Still no iteration over behaviors, but assuming there's those specific two ones that exist at the moment
- Selecting the behavior should be possible in regular ruledit build, not requiring experimental one

+void tab_counter::counter_behaviour_selected(int item)
+{
+ (void) item;

- We don't have warnings about unused parameters enabled on any environment, so that "(void) item;" should not be needed. The problem with it is that if we are going to make some special "unused parameters" arrangement in the future, it hides this one

- Empty line missing after "QVariant item_data;"
- Indentation in refresh() off
- delete_now() compares 'selected' against 0, not nullptr

+ || pcount->ruledit_disabled) {

- Extra space between "||" and "pcount->...", indentation of the line off

+ //QMenu *prepare_counter_button(QToolButton *button, enum tech_req rn);

- There's no need to keep this commented-out line at all

Reply To cazfi

- slots for checkpoint and default value do not check if there's a counter selected. Program crashes if one just goes to Counters tab and touches those widgets without selecting a counter
- Coding Style of add_widget() and add_widget_next() macros is really bad (I don't particularly like them in general, but they are acceptable)
- Still no iteration over behaviors, but assuming there's those specific two ones that exist at the moment
- Selecting the behavior should be possible in regular ruledit build, not requiring experimental one
-- +void tab_counter::counter_behaviour_selected(int item)
+{
+ (void) item;
- We don't have warnings about unused parameters enabled on any environment, so that "(void) item;" should not be needed. The problem with it is that if we are going to make some special "unused parameters" arrangement in the future, it hides this one
-- - Empty line missing after "QVariant item_data;"
- Indentation in refresh() off
- delete_now() compares 'selected' against 0, not nullptr
-- + || pcount->ruledit_disabled) { - Extra space between "||" and "pcount->...", indentation of the line off
-- + //QMenu *prepare_counter_button(QToolButton *button, enum tech_req rn); - There's no need to keep this commented-out line at all

I do not see unnecessary space in refresh() . Maybe do you see older patch?

- widgets_row() macro has style issues bad enough to cause meson/clang compile to fail. Just correcting the coding style should fix this; "Use {} after if () even for a single line"

../../../../src/tools/ruledit/tab_counters.cpp:57:35: error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]

../../../../src/tools/ruledit/tab_counters.cpp:57:35: note: in definition of macro ‘widgets_row’

../../../../src/tools/ruledit/tab_counters.cpp:61:37: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’

../../../../src/tools/ruledit/tab_counters.cpp:61:37: note: in definition of macro ‘widgets_row’

And as you still need to make a new version to fix that compile failure, should fix also these:

- "{" is misplaced also after "for (unsigned int i = 0; i < sizeof(wid) / sizeof(wid[0]); i++)" - should be on the same line

- What is "+#include <QtGlobal>" needed for? If really needed, it's not in right place alphabetically
- "+#include <QToolButton>" seems extraneous too - No QToolButton used in the file.
- Indentation in refresh() off - 4 spaces instead of 2 within the iterator

Reply To cazfi

- widgets_row() macro has style issues bad enough to cause meson/clang compile to fail. Just correcting the coding style should fix this; "Use {} after if () even for a single line"
../../../../src/tools/ruledit/tab_counters.cpp:57:35: error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
57 | if (wid[i]) \
| ^~
../../../../src/tools/ruledit/tab_counters.cpp:57:35: note: in definition of macro ‘widgets_row’
57 | if (wid[i]) \
| ^~
../../../../src/tools/ruledit/tab_counters.cpp:61:37: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
61 | column++;\
| ^~
../../../../src/tools/ruledit/tab_counters.cpp:61:37: note: in definition of macro ‘widgets_row’
61 | column++;\
----

I have found this page demonstrating how to use meson with clang: https://gensoft.pasteur.fr/docs/mesa/19.0.8/meson.html . I do:

No errors. Maybe should I change buildtype?

Sorry, I should have record more information about the environment where the build failed.

Actually I was wrong even about "clang". I assumed that clang is the default for meson on that environment, but now that I checked the detais it's actually gcc-12.2. (meson is 1.0.0, but that's unlikely to be relevant). In general that's Debian Bookworm, on x86-64.