Skip to content

Conversation

@ptziegler
Copy link
Contributor

This adds a new AsyncReadyCallback class which is used to handle the asynchronous execution of dialogs. The goal is provide a cleaner and more readable interface than what is currently available by SyncDialogUtil.

Note that this class currently simply wraps the call to SyncDialogUtil. But once all of the remaining dialogs (Color/Font/MessageDialog) have been migrated, it might make sense to remove this class entirely to avoid this additional indirection.

Follow-up to 2e61b4b

@ptziegler
Copy link
Contributor Author

ptziegler commented Nov 9, 2024

Motivation being that I started on migrating the FontDialog and I had a hard time understanding what those two lambda expressions in SyncDialogUtil.run() are supposed to do. Hopefully this approach is a little bit more readable.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2024

Test Results

   483 files  ±0     483 suites  ±0   8m 25s ⏱️ -55s
 4 095 tests ±0   4 085 ✅ ±0   7 💤 ±0  3 ❌ ±0 
16 173 runs  ±0  16 080 ✅ ±0  90 💤 ±0  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit e4f9857. ± Comparison against base commit 45405e5.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Contributor

laeubi commented Nov 9, 2024

It might be good to delay the merge of this until we enabled (hopefully soon) the compilation of GTK4 so we do not run into issues with newer GTK API.

* transform an asynchronous {@code async} and synchronous {@code await}
* operation into a single synchronous {@code run} operation.
*/
public abstract class AsyncReadyCallback {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make this an interface and put the run method into SyncDialogUtils (or make it default implemented)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I'd really like to get rid of the SyncDialogUtils class, which is why I didn't move the run method there. But the best way can also be decided at a later stage, I suppose...
In any case, this class is now an interface and the old run() method in SyncDialogUtils has been replaced.

@ptziegler ptziegler force-pushed the gtk4-sync-dialog-util branch 3 times, most recently from cdba6b9 to d690c09 Compare November 11, 2024 16:30
@ptziegler
Copy link
Contributor Author

It might be good to delay the merge of this until we enabled (hopefully soon) the compilation of GTK4 so we do not run into issues with newer GTK API.

Wouldn't this be more relevant for #1583 given, that I don't change the native bindings here? Or would this then also execute the tests in a GTK4 environment?

@akurtakov akurtakov force-pushed the gtk4-sync-dialog-util branch from d690c09 to a044e15 Compare November 26, 2024 18:43
@akurtakov
Copy link
Member

Would you please share how you envision getting rid of SyncDialogUtils? Without a bit longer term strategy info this PR looks like syntax change only.

@ptziegler
Copy link
Contributor Author

Would you please share how you envision getting rid of SyncDialogUtils? Without a bit longer term strategy info this PR looks like syntax change only.

The SyncDialogUtils is currently only used for GTK4 and only for the dialogs.

image

It currently only defines the following methods:

  • static public long run(Display display, Consumer<Long> asyncOpen, Function<Long, Long> asyncFinish)
  • static public int run(Display display, long handle, boolean isNativeDialog)

The former is what's being replaced by this PR and the method that's used for the migrated GTK4 dialogs, while other one is used by the other (GTK4) dialogs.

The way the latter works is that it opens the dialog and then blocks until it is closed again. Afterwards, the dialog result is fetched. This approach doesn't really work with the new dialogs, as the result can only be fetched while the dialog closes and only within a callback method, which is why I initially added the other method.

This also means that once the other dialogs have been migrated, the old method is no longer needed and can be removed. With only one other method left, I would then simply move the callback mechanism into the AsyncReadCallback class and delete the SyncDialogUtil class alltogether, in order to remove this additional indirection.

This adds a new AsyncReadyCallback class which is used to handle the
asynchronous execution of dialogs. The goal is provide a cleaner and
more readable interface than what is currently available by
SyncDialogUtil.

Note that this class currently simply wraps the call to SyncDialogUtil.
But once all of the remaining dialogs (Color/Font/MessageDialog) have
been migrated, it might make sense to remove this class entirely to
avoid this additional indirection.

Follow-up to 2e61b4b
@akurtakov akurtakov force-pushed the gtk4-sync-dialog-util branch from a044e15 to e4f9857 Compare November 27, 2024 17:18
@akurtakov
Copy link
Member

Thanks for the explanation. Sounds like a good plan.

@akurtakov akurtakov merged commit 2cc00b7 into eclipse-platform:master Nov 27, 2024
8 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants