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.

Sign up for GitHub

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

Jump to bottom

Add location test to demo #72

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add location test to demo #72

wants to merge 1 commit into from

Conversation

A6GibKm
Copy link
Contributor

@A6GibKm A6GibKm commented Jan 12, 2022 •
edited
Loading

There is a caveat to test this in the demo, it has to be installed for it to work.

GeorgesStavracas
GeorgesStavracas requested changes Jan 12, 2022
View reviewed changes
portal-test/gtk4/window.ui Outdated
</object>
</child>
<child>
<object class="GtkLabel" id="lat">
Copy link
Member

Choose a reason for hiding this comment

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

latitudeLabel

Copy link
Member

Choose a reason for hiding this comment

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

Neither this

portal-test/gtk4/window.ui Outdated
</object>
</child>
<child>
<object class="GtkLabel" id="lon">
Copy link
Member

Choose a reason for hiding this comment

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

longitudeLabel

Copy link
Member

Choose a reason for hiding this comment

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

Neither this

libportal/location.c Outdated
@@ -185,6 +185,8 @@ call_returned (GObject *object,
g_task_return_error (call->task, error);
create_call_free (call);
}
else
ensure_location_updated_connected (call->portal);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ensure_location_updated_connected (call->portal);
{
ensure_location_updated_connected (call->portal);
}

Copy link
Member

Choose a reason for hiding this comment

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

This apparently has not been resolved

libportal/updates.c Outdated
Comment on lines 345 to 348
{
ensure_update_monitor_connection (call->portal);
g_task_return_boolean (call->task, TRUE);
}
Copy link
Member

Choose a reason for hiding this comment

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

  if (error)
    {
      g_task_return_error (call->task, error);
    }
  else
   {
     ensure_update_monitor_connection (call->portal);
     g_task_return_boolean (call->task, TRUE);
   }

Copy link
Member

Choose a reason for hiding this comment

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

Neither this one

@smcv
Copy link
Contributor

smcv commented Jan 12, 2022

If done too soon it won't work

In what way does it not work?

I'm cautious about this because delaying a D-Bus signal connection until after a successful method call often leads to this anti-pattern:

  • client: call GetThing()
  • server: reply GetThing() -> 23
  • server: emit ThingChanged(to: 42)
  • client: receive reply GetThing() -> 23
  • client: subscribe to ThingChanged
  • too late! client will never find out that Thing changed to 42

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Jan 12, 2022

If done too soon it won't work

In what way does it not work?

I'm cautious about this because delaying a D-Bus signal connection until after a successful method call often leads to this anti-pattern:

* client: call GetThing()

* server: reply GetThing() -> 23

* server: emit ThingChanged(to: 42)

* client: receive reply GetThing() -> 23

* client: subscribe to ThingChanged

* too late! client will never find out that Thing changed to 42

It does not work in the sense that LocationUpdated will never be emitted on the other side of the DBus.

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Jan 12, 2022

I am not convinced this is the solution, but before this changes the location portal would fail about 19/20 times, now it only fails about 1/8 of times.

@A6GibKm A6GibKm changed the title Fixes location and updates signal emissions WIP: Fixes location and updates signal emissions Jan 12, 2022
@A6GibKm A6GibKm marked this pull request as draft January 12, 2022 14:32
@A6GibKm
Copy link
Contributor Author

A6GibKm commented Jan 12, 2022

I am marking a draft for if someone has a better solution.

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Jan 12, 2022

I think ill close it, it does not work on my other machine. This issue is the weirdest condition race I have ever seen.

All I know this issue is in libportal side since, ASHPD works in a reliable way.way.

@grulja
Copy link
Contributor

grulja commented Jan 13, 2022

I believe this can be closed as #67 is closed now.

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Jan 13, 2022

I updated the MR so just to include the location portal test to the demo.

@A6GibKm A6GibKm changed the title WIP: Fixes location and updates signal emissions Add location test to demo Jan 13, 2022
@A6GibKm A6GibKm marked this pull request as ready for review January 13, 2022 12:17
GeorgesStavracas
GeorgesStavracas requested changes Jun 20, 2022
View reviewed changes
libportal/location.c Outdated
@@ -185,6 +185,8 @@ call_returned (GObject *object,
g_task_return_error (call->task, error);
create_call_free (call);
}
else
ensure_location_updated_connected (call->portal);
Copy link
Member

Choose a reason for hiding this comment

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

This apparently has not been resolved

libportal/updates.c Outdated
Comment on lines 345 to 348
{
ensure_update_monitor_connection (call->portal);
g_task_return_boolean (call->task, TRUE);
}
Copy link
Member

Choose a reason for hiding this comment

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

Neither this one

portal-test/gtk4/window.ui Outdated
</object>
</child>
<child>
<object class="GtkLabel" id="lon">
Copy link
Member

Choose a reason for hiding this comment

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

Neither this

portal-test/gtk4/window.ui Outdated
</object>
</child>
<child>
<object class="GtkLabel" id="lat">
Copy link
Member

Choose a reason for hiding this comment

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

Neither this

@A6GibKm
docs: Add favicons
734524b 
See https://gitlab.gnome.org/GNOME/gi-docgen/-/merge_requests/201.
Generated by
https://gitlab.gnome.org/World/design/emblem/-/merge_requests/40.

The icon used was portal-symbolic from the devkit, using colors #26A269
to #33D17A were taken from the source of the SVG.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgesStavracas GeorgesStavracas GeorgesStavracas requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@A6GibKm @smcv @grulja @GeorgesStavracas