Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Feature #846914

Task #774803: Emscripten clients for web.

Remove nested Qt event loops

Added by Zoltán Žarkov over 2 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Category:
gui-qt
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

Qt client uses .exec() all over the place. Emscripten qt does not allow nested event loops, so refactoring to enable.


Related issues

Related to Freeciv - Feature #851519: CodingStyle: Allowing C++11 lambdas in C++ codeClosed

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>

History

#2 Updated by Marko Lindqvist over 2 years ago

Is this really needed for S2_6?

#3 Updated by Zoltán Žarkov over 2 years ago

Maybe ok to ignore the S2_6 patch. We may run at least one 2.6 cross-platform game before 3.0 releases are stable.

#4 Updated by Sveinung Kvilhaugsvik over 2 years ago

  • Assignee deleted (Sveinung Kvilhaugsvik)

Sveinung, do you have time for this one?

Probably not in the nearest future. But I'm not a planner. So I may suddenly find or take time to do it. Setting assignee to blank so someone else can take it if they have time.

#5 Updated by Marko Lindqvist over 2 years ago

  • Category set to gui-qt
  • Sprint/Milestone changed from 2.6.1 to 3.0.0

#6 Updated by Marko Lindqvist over 2 years ago

This introduces callback functions inside the body of the other function. Our CodingStyle has nothing to say about that, so I'll give people a bit time to comment.

#7 Updated by Marko Lindqvist over 2 years ago

  • Tracker changed from Task to Feature
  • Status changed from New to Resolved
  • Assignee set to Marko Lindqvist

#8 Updated by Zoltán Žarkov over 2 years ago

Note: Avoid using this function; instead, use open(). Unlike exec(), open() is asynchronous, and does not spin an additional event loop.

https://doc.qt.io/qt-5/qdialog.html#exec

Up for discussion which C++11 features we want to allow, but I think lambdas make the code more readable here.

#9 Updated by Louis Moureaux over 2 years ago

C++11 lambdas are the way to go in this context. Their use is also encouraged by Qt.

However, one should be very careful when capturing pointers, since the pointed-to memory may be freed between capture and execution. It looks like the following code may lead to a crash:

QObject::connect(ask, &hud_message_box::finished, [=](int ret) {
  if (ret == QMessageBox::Ok) {
    if (!pcity->did_sell && city_has_building(pcity, building)) {
      city_sell_improvement(pcity, improvement_number(building));
    }
  }
});

Consider the following sequence:

  • Start selling building in city; confirmation popup appears
  • City is destroyed, freeing *pcity
  • Confirmation popup is accepted and tries to access *pcity

I say allow lambdas but discourage pointer capture.

#10 Updated by Marko Lindqvist over 2 years ago

  • Status changed from Resolved to In Progress
  • Assignee changed from Marko Lindqvist to Zoltán Žarkov

Good catch, Louis.

#11 Updated by Marko Lindqvist over 2 years ago

  • Related to Feature #851519: CodingStyle: Allowing C++11 lambdas in C++ code added

#12 Updated by Marko Lindqvist over 2 years ago

As per Feature #851519 findings, only master version should use lambdas. S3_0 version should not, assuming this patch is wanted there.

#13 Updated by Zoltán Žarkov over 2 years ago

I have all nested execs removed here, but I can break this patch into smaller reviewable chunks if needed. Will target only master, because lambdas make this much easier.

#14 Updated by Marko Lindqvist over 2 years ago

  • Status changed from In Progress to Resolved

#15 Updated by Louis Moureaux over 2 years ago

    QObject::connect(ask, &hud_message_box::finished, [=](int ret) {
  QObject::connect(ask, &hud_message_box::finished, [=](int ret) {

Maybe using accepted() would be cleaner than checking ret.
    int which_menu = qvar.toInt();
    int c_id = qvar.toInt();

Isn't this is different from
    which_menu = qvar.toInt();
    qvar = act->data();
    c_id = qvar.toInt();

(notice how qvar changes)
            QObject::connect(ask, &hud_message_box::finished, [=](int ret) {
              if (ret == QMessageBox::Ok) {
                if (!pcity->did_sell && city_has_building(pcity, building)) {
                  city_sell_improvement(pcity, improvement_number(building));
                }
              }
        });

Missed conversion? pcity is still used in the lambda.
    Q_ASSERT(col >= 0);

Worth changing to fc_assert_ret? Q_ASSERT will exit.
  diplomat_queue_handle_secondary(diplomat_id);

Shouldn't it be moved inside the handlers?
void eco_report::sell_buildings()

{

Extra blank line added.

#16 Updated by Louis Moureaux over 2 years ago

FTR two possible bugs found while reviewing this patch:

[14:24] <louis94> I notice that city_dialog keeps a pointer to the city struct
[14:24] <louis94> This is probably fine most of the time but in some cases not
[14:26] <louis94> Think city is removed, city dialog is popdown is triggered and struct is deleted, but there was still something queued in the event loop
[14:26] <louis94> In this case I think Qt will still deliver the event before popdown
[14:26] <louis94> But by then the struct will be gone...
[14:27] <louis94> I didn't do anything to confirm this and don't even have a clear idea how

[14:42] <louis94> Also, I see char* being passed to Qt, no encoding problem there? In the other direction we use toLocal8Bits()

#17 Updated by Marko Lindqvist over 2 years ago

  • Status changed from Resolved to In Progress

#19 Updated by Louis Moureaux over 2 years ago

Looks good to me, ship it!

#20 Updated by Marko Lindqvist over 2 years ago

  • Status changed from In Progress to Resolved

#21 Updated by Marko Lindqvist over 2 years ago

  • Assignee changed from Zoltán Žarkov to Marko Lindqvist

#22 Updated by Marko Lindqvist over 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF