fix: fallback implementation is used if preferred value of none #1255

SoumyaRanjanPatnaik · 2023-12-31T14:24:03Z

Don't use fallback portal implementations in the following cases:

the preferred portals for an interface contains none
no preference is set for the interface but the default preference contains none

Fixes #1254

ebassi

Looks good to me.

src/portal-impl.c

SoumyaRanjanPatnaik · 2024-01-04T11:02:58Z

Fixed lint issues :)

GreyXor · 2024-01-11T15:49:43Z

Hi, thanks for this PR. that would be nice to have this fix in the next release. That could fix sway+firefox idle inhibit https://gitlab.archlinux.org/archlinux/packaging/packages/sway/-/issues/2

jp7677 · 2024-02-09T07:33:36Z

Any chance to get this PR merged?
(That said, tests are failing, may be some additional work on that front is needed.)

This would fix the firefox-idle-inhibit issue not only for sway, but for other/all wlroots based compositors, labwc in my case.

GeorgesStavracas · 2024-02-09T10:55:09Z

Not only tests must pass, but the commits have to be squashed, and the commit message must follow the pattern that other commits do.

SoumyaRanjanPatnaik · 2024-02-09T11:00:03Z

@GeorgesStavracas The tests don't fail on my system. I can't figure out why they fail in CI.

$ meson test -C build

ninja: Entering directory `/home/soumyarp/git/xdg-desktop-portal/build'
[61/61] Linking target tests/limited-portals
 1/24 xdg-desktop-portal / testdb                                    OK              0.01s   3 subtests passed
 2/24 xdg-desktop-portal / test-doc-portal                           OK              0.10s   5 subtests passed
 3/24 xdg-desktop-portal:portals / test-portals-camera               OK              1.10s   1 subtests passed
 4/24 xdg-desktop-portal:portals / test-portals-background           OK              1.10s   1 subtests passed
 5/24 xdg-desktop-portal:portals / test-portals-account              OK              1.11s   1 subtests passed
 6/24 xdg-desktop-portal:portals / test-portals-inhibit              OK              1.11s   1 subtests passed
 7/24 xdg-desktop-portal:portals / test-portals-notification         OK              1.10s   1 subtests passed
 8/24 xdg-desktop-portal:portals / test-portals-location             SKIP            1.11s   0 subtests passed
 9/24 xdg-desktop-portal:portals / test-portals-email                OK              1.11s   1 subtests passed
10/24 xdg-desktop-portal:portals / test-portals-color                SKIP            1.12s
11/24 xdg-desktop-portal:portals / test-portals-openfile             SKIP            1.10s
12/24 xdg-desktop-portal:portals / test-portals-prepareprint         SKIP            1.11s
13/24 xdg-desktop-portal:portals / test-portals-savefile             SKIP            1.10s
14/24 xdg-desktop-portal:portals / test-portals-openuri              OK              1.12s   1 subtests passed
15/24 xdg-desktop-portal:portals / test-portals-print                OK              1.11s   1 subtests passed
16/24 xdg-desktop-portal:portals / test-portals-screenshot           OK              1.10s   1 subtests passed
17/24 xdg-desktop-portal:portals / test-portals-settings             OK              1.09s   1 subtests passed
18/24 xdg-desktop-portal:portals / test-portals-trash                OK              1.09s   1 subtests passed
19/24 xdg-desktop-portal:portals / limited-portals-openfile          SKIP            1.05s
20/24 xdg-desktop-portal:portals / limited-portals-savefile          SKIP            1.05s
21/24 xdg-desktop-portal:portals / test-portals-wallpaper            OK              1.07s   1 subtests passed
22/24 xdg-desktop-portal / test-permission-store                     OK              0.08s   14 subtests passed
23/24 xdg-desktop-portal / test-xdp-utils                            OK              0.01s   6 subtests passed
24/24 xdg-desktop-portal / test-xdp-method-info                      OK              0.00s   2 subtests passed

Ok:                 17  
Expected Fail:      0   
Fail:               0   
Unexpected Pass:    0   
Skipped:            7   
Timeout:            0   

Full log written to /home/soumyarp/git/xdg-desktop-portal/build/meson-logs/testlog.txt

GeorgesStavracas · 2024-02-09T11:01:09Z

Maybe a simple rebase would be sufficient? Please make sure to update the commit message

SoumyaRanjanPatnaik · 2024-02-09T11:07:47Z

Squashed the commits and rebased on top of main. Hope the tests check out.

jp7677 · 2024-02-09T12:54:19Z

@SoumyaRanjanPatnaik CI test failures shows:

 19/30 xdg-desktop-portal:portals / limited-portals-openfile           ERROR            0.28s   killed by signal 6 SIGABRT
>>> G_TEST_SRCDIR=/src/tests MALLOC_PERTURB_=229 XDG_CURRENT_DESKTOP=limited XDP_UNINSTALLED=1 XDG_DATA_DIRS=/src/builddir/tests/share G_DEBUG=gc-friendly G_TEST_BUILDDIR=/src/builddir/tests /src/builddir/tests/limited-portals --verbose --keep-going --tap -p /limited/openfile

[20-21/30] 🌒 xdg-desktop-portal:portals / limited-portals-savefile                    0/ 30s
20/30 xdg-desktop-portal:portals / limited-portals-savefile           ERROR            0.25s   killed by signal 6 SIGABRT
>>> G_TEST_SRCDIR=/src/tests XDG_CURRENT_DESKTOP=limited XDP_UNINSTALLED=1 XDG_DATA_DIRS=/src/builddir/tests/share G_DEBUG=gc-friendly G_TEST_BUILDDIR=/src/builddir/tests MALLOC_PERTURB_=217 /src/builddir/tests/limited-portals --verbose --keep-going --tap -p /limited/savefile

You don't hit those errors locally because your test setup skipped those tests. From your logs:

19/24 xdg-desktop-portal:portals / limited-portals-openfile          SKIP            1.05s
20/24 xdg-desktop-portal:portals / limited-portals-savefile          SKIP            1.05s

SoumyaRanjanPatnaik · 2024-02-09T21:50:45Z

I don't want to be that guy, but either failing tests are wrong. Either that or the FileChooser interface has a bug. Let me elaborate...

Previously, with none not working, the limited-portals-openfile and limited-portals-savefile tests probably called an interface that is not enabled in the limited-portals.conf file.
This patch caused none to function correctly, which caused the calls to the disabled interface to fail.
Setting the default interface to limited in limited-portals.conf causes the failing test cases to pass

I have yet to investigate which interfaces are being called in these tests. The naive fix is just to set the default interface to limited, which I don't think might be a very good idea.

[preferred]
- default=none
+ default=limited
org.freedesktop.impl.portal.Account=limited
org.freedesktop.impl.portal.FileChooser=limited
org.freedesktop.impl.portal.Lockdown=limited
org.freedesktop.impl.portal.Settings=limited

The alternative is to either figure out if the disabled interfaces are invoked and remove occurrences of these invocations, or to fix the FileChooser interface to not fail when the default backend is none.

Since fixing tests or the FileChooser interface is unrelated to this PR, it is probably better to file a separate issue.

SoumyaRanjanPatnaik · 2024-02-09T22:00:23Z

@jp7677 thanks for pointing out the failing tests were skipped on my system. It turns out I had to enable the installed-tests feature for those tests to not be skipped on my system. It might be a good idea to add this to the contributor documentation.

This is how I ran the tests:

meson setup . build --prefix /usr -Dinstalled-tests=true
meson test -C build

jp7677 · 2024-02-10T17:19:17Z

Hi @SoumyaRanjanPatnaik I'm looking (without having former experience with this project) at your patch and the tests.

Your patch says:

static gboolean
portal_interface_prefers_none (const char *interface)
{
  const PortalInterface *iface = find_matching_iface_config (interface);

  if (iface != NULL && g_strv_contains ((const char * const *) iface->portals, "none"))
    {
      g_debug ("Found 'none' in configuration for %s", iface->dbus_name);
      return TRUE;
    }

  if (config != NULL && g_strv_contains ((const char * const *) config->default_portal->portals, "none"))
      return TRUE;

  return FALSE;
}

As far as I'm reading this, it translates to: disable this interface when the interface itself is disabled (org.freedesktop.impl.portal.FileChooser=none) or no fallback is set (default=none).

Looking at the test configuration:

[preferred]
default=none
org.freedesktop.impl.portal.Account=limited
org.freedesktop.impl.portal.FileChooser=limited
org.freedesktop.impl.portal.Lockdown=limited
org.freedesktop.impl.portal.Settings=limited

this means that all interfaces are disabled because one of the two conditions (the second one) is true. I think (not sure though) that the second if statement should only return true when that specific interface is not mentioned in the configuration file (or something similar). So I think the test failure is justified.

Edit: May be something like this is needed (essentially adding iface == NULL as condition to your version), the tests are good with this change and the test debug output shows expected output when I'm playing with 'none':

diff --git a/src/portal-impl.c b/src/portal-impl.c
index 7086f55..1c6e6b3 100644
--- a/src/portal-impl.c
+++ b/src/portal-impl.c
@@ -516,8 +516,11 @@ portal_interface_prefers_none (const char *interface)
       return TRUE;
     }
 
-  if (config != NULL && g_strv_contains ((const char * const *) config->default_portal->portals, "none"))
-    return TRUE;
+  if (iface == NULL && config != NULL && g_strv_contains ((const char * const *) config->default_portal->portals, "none"))
+    {
+      g_debug ("No configuration found for %s and default is 'none'", interface);
+      return TRUE;
+    }
 
   return FALSE;
 }

SoumyaRanjanPatnaik · 2024-02-11T12:11:09Z

As far as I'm reading this, it translates to: disable this interface when the interface itself is disabled (org.freedesktop.impl.portal.FileChooser=none) or no fallback is set (default=none).

@jp7677 Thanks for correctly pointing this out. I prematurely blamed the tests (really stupid of me 😢). Tests seem to pass after updating portal_interface_prefers_none with the following.

static gboolean
portal_default_prefers_none ()
{
  if (config != NULL && g_strv_contains ((const char * const *) config->default_portal->portals, "none"))
    {
      g_debug ("Found 'none' in configuration for default");
      return TRUE;
    }

  return FALSE;
}

static gboolean
portal_interface_prefers_none (const char *interface)
{
  const PortalInterface *iface = find_matching_iface_config (interface);
  if (iface == NULL)
    return portal_default_prefers_none();

  if (g_strv_contains ((const char * const *) iface->portals, "none"))
    {
      g_debug ("Found 'none' in configuration for %s", iface->dbus_name);
      return TRUE;
    }

  return FALSE;
}

jp7677 · 2024-02-11T12:55:40Z

@SoumyaRanjanPatnaik no worries, I’m happy to help to bring this forward. Thanks a lot for your work in the first place to address this issue!
(I guess #1282 can be closed now ;) )

jp7677 · 2024-02-23T07:53:52Z

@GeorgesStavracas @ebassi is there anything left that needs to be done for this commit to get in? FWIW, I'm running this commit on top of xdg-desktop-portal-1.18.2 together with labwc for more than a week now and it works as intended without noticing any negative side effects.

src/portal-impl.c

Don't use fallback portal implementations in the following cases: - the preferred portals for an interface contains `none` - no preference is set for the interface but the default preference contains `none`

SoumyaRanjanPatnaik · 2024-02-23T20:28:24Z

@GeorgesStavracas Thanks for the review. Pushed the changes.

SoumyaRanjanPatnaik · 2024-02-23T20:33:51Z

Please follow the project code style:

Just curious, does this project have any documentation or formatter for enforcing the coding style? I searched for some documentation on code but found no leads in this regard.

jp7677 · 2024-03-08T06:53:22Z

@ebassi @GeorgesStavracas gentle ping...

GeorgesStavracas · 2024-03-08T16:27:28Z

Thanks

SoumyaRanjanPatnaik mentioned this pull request Dec 31, 2023

UseIn fallbck is used even if preference is set to none for interfaces within portals.conf file #1254

Closed

GeorgesStavracas added bug config Issues with the portal configuration mechanism labels Jan 3, 2024

GeorgesStavracas requested a review from ebassi January 3, 2024 19:12

ebassi reviewed Jan 3, 2024

View reviewed changes

src/portal-impl.c Outdated Show resolved Hide resolved

src/portal-impl.c Outdated Show resolved Hide resolved

src/portal-impl.c Outdated Show resolved Hide resolved

src/portal-impl.c Outdated Show resolved Hide resolved

SoumyaRanjanPatnaik requested a review from ebassi January 4, 2024 11:02

ebassi approved these changes Jan 16, 2024

View reviewed changes

SoumyaRanjanPatnaik force-pushed the fix/none_preference branch from 37d02f8 to 497b28d Compare February 9, 2024 11:06

SoumyaRanjanPatnaik mentioned this pull request Feb 9, 2024

Incorrect tests - limited-portals-openfile and limited-portals-savefile or bug in FileChooser interface #1282

Closed

SoumyaRanjanPatnaik force-pushed the fix/none_preference branch from 497b28d to 82c3819 Compare February 11, 2024 12:07

SoumyaRanjanPatnaik requested a review from ebassi February 11, 2024 12:11

GeorgesStavracas reviewed Feb 23, 2024

View reviewed changes

src/portal-impl.c Outdated Show resolved Hide resolved

GeorgesStavracas reviewed Feb 23, 2024

View reviewed changes

src/portal-impl.c Outdated Show resolved Hide resolved

GeorgesStavracas reviewed Feb 23, 2024

View reviewed changes

src/portal-impl.c Outdated Show resolved Hide resolved

portal-impl: fallback implementation is used if preferred value is none

c15ba89

Don't use fallback portal implementations in the following cases: - the preferred portals for an interface contains `none` - no preference is set for the interface but the default preference contains `none`

SoumyaRanjanPatnaik force-pushed the fix/none_preference branch from 82c3819 to c15ba89 Compare February 23, 2024 20:27

SoumyaRanjanPatnaik requested a review from GeorgesStavracas February 23, 2024 20:37

GeorgesStavracas approved these changes Mar 8, 2024

View reviewed changes

GeorgesStavracas added this to the 1.20 milestone Mar 8, 2024

GeorgesStavracas added this pull request to the merge queue Mar 8, 2024

Merged via the queue into flatpak:main with commit 1394bb2 Mar 8, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fallback implementation is used if preferred value of none #1255

fix: fallback implementation is used if preferred value of none #1255

SoumyaRanjanPatnaik commented Dec 31, 2023

ebassi left a comment

SoumyaRanjanPatnaik commented Jan 4, 2024

GreyXor commented Jan 11, 2024

jp7677 commented Feb 9, 2024

GeorgesStavracas commented Feb 9, 2024

SoumyaRanjanPatnaik commented Feb 9, 2024

GeorgesStavracas commented Feb 9, 2024

SoumyaRanjanPatnaik commented Feb 9, 2024

jp7677 commented Feb 9, 2024

SoumyaRanjanPatnaik commented Feb 9, 2024

SoumyaRanjanPatnaik commented Feb 9, 2024 •

edited

jp7677 commented Feb 10, 2024 •

edited

SoumyaRanjanPatnaik commented Feb 11, 2024 •

edited

jp7677 commented Feb 11, 2024

jp7677 commented Feb 23, 2024

SoumyaRanjanPatnaik commented Feb 23, 2024

SoumyaRanjanPatnaik commented Feb 23, 2024 •

edited

jp7677 commented Mar 8, 2024

GeorgesStavracas commented Mar 8, 2024

fix: fallback implementation is used if preferred value of none #1255

fix: fallback implementation is used if preferred value of none #1255

Conversation

SoumyaRanjanPatnaik commented Dec 31, 2023

ebassi left a comment

Choose a reason for hiding this comment

SoumyaRanjanPatnaik commented Jan 4, 2024

GreyXor commented Jan 11, 2024

jp7677 commented Feb 9, 2024

GeorgesStavracas commented Feb 9, 2024

SoumyaRanjanPatnaik commented Feb 9, 2024

GeorgesStavracas commented Feb 9, 2024

SoumyaRanjanPatnaik commented Feb 9, 2024

jp7677 commented Feb 9, 2024

SoumyaRanjanPatnaik commented Feb 9, 2024

SoumyaRanjanPatnaik commented Feb 9, 2024 • edited

jp7677 commented Feb 10, 2024 • edited

SoumyaRanjanPatnaik commented Feb 11, 2024 • edited

jp7677 commented Feb 11, 2024

jp7677 commented Feb 23, 2024

SoumyaRanjanPatnaik commented Feb 23, 2024

SoumyaRanjanPatnaik commented Feb 23, 2024 • edited

jp7677 commented Mar 8, 2024

GeorgesStavracas commented Mar 8, 2024

SoumyaRanjanPatnaik commented Feb 9, 2024 •

edited

jp7677 commented Feb 10, 2024 •

edited

SoumyaRanjanPatnaik commented Feb 11, 2024 •

edited

SoumyaRanjanPatnaik commented Feb 23, 2024 •

edited