Skip to content

Conversation

@TheEvilSkeleton
Copy link
Contributor

@TheEvilSkeleton TheEvilSkeleton commented Mar 26, 2025

Admittedly, it took me a good 30 minutes to understand why I couldn't use the instance methods for Xdp.Settings, which required me to dig through libportal's source code and realize that these methods aren't part of libportal/portal.h. The solution is to simply include libportal/settings.h.


  • Steps to reproduce: #include <libportal/portal.h> (only) and try to call instance methods on XdpSettings
  • Expected result: can do so
  • Actual result: compiler errors because settings.h wasn't included

I also wrote a small reproducer: TheEvilSkeleton/Settings-Not-Included@8eb0714

@TheEvilSkeleton TheEvilSkeleton force-pushed the expose-all-headers-in-docs branch from 34339b6 to 13d7f2e Compare March 30, 2025 22:25
@ebassi
Copy link

ebassi commented Apr 16, 2025

It's a bit naff, to be honest. I don't understand why there isn't a single header include for libportal, just like every other C library that depends on GLib. Single header inclusion makes it possible to move type declarations around without breaking source compatibility.

@smcv
Copy link
Contributor

smcv commented Apr 16, 2025

I don't understand why there isn't a single header include for libportal, just like every other C library that depends on GLib

There is, <libportal/portal.h>, but from @TheEvilSkeleton's report it seems that it's incomplete.

If fixing this is going to require a libportal source-code change anyway, probably including all of the libportal core headers in portal.h would be a better answer to this than changing the introspection?

(but not the GTK and Qt addons, those need to stay separate!)

@smcv
Copy link
Contributor

smcv commented Apr 16, 2025

Admittedly, it took me a good 30 minutes to understand why I couldn't use the instance methods for Xdp.Settings, which required me to dig through libportal's source code and realize that these methods aren't part of libportal/portal.h. I believe exposing all the public headers would help developers who want to use libportal.

It sounds like you're implicitly reporting a bug. What is the bug? It's usually better to describe the bug than making maintainers reverse-engineer it from a proposed fix, particularly in projects that are limited by maintainer bandwidth.

Presumably something like this:

  • Steps to reproduce: #include <libportal/portal.h> (only) and try to call instance methods on XdpSettings (?)
  • Expected result: can do so
  • Actual result: compiler errors because settings.h wasn't included

@TheEvilSkeleton
Copy link
Contributor Author

It sounds like you're implicitly reporting a bug. What is the bug? It's usually better to describe the bug than making maintainers reverse-engineer it from a proposed fix, particularly in projects that are limited by maintainer bandwidth.

True, sorry

TheEvilSkeleton added a commit to TheEvilSkeleton/Settings-Not-Included that referenced this pull request Apr 16, 2025
@TheEvilSkeleton
Copy link
Contributor Author

  • Steps to reproduce: #include <libportal/portal.h> (only) and try to call instance methods on XdpSettings (?)

  • Expected result: can do so

  • Actual result: compiler errors because settings.h wasn't included

Yes, it's pretty much that. I also wrote a reproducer in this commit: TheEvilSkeleton/Settings-Not-Included@8eb0714

@smcv
Copy link
Contributor

smcv commented Apr 16, 2025

OK, thanks for the reproducer. I think that confirms my belief that a better fix for this would be to add #include <libportal/settings.h> to portal.h so that you don't have to know about the individual headers.

Are there any other public headers that are not included in portal.h? We should add those too, unless there's a reason they're inappropriate (for example the GTK and Qt headers, which have extra dependencies, and are mutually exclusive between major versions of the same library).

@TheEvilSkeleton
Copy link
Contributor Author

Are there any other public headers that are not included in portal.h? We

Not that I'm aware of, no

@TheEvilSkeleton TheEvilSkeleton force-pushed the expose-all-headers-in-docs branch from 13d7f2e to 6ef48ed Compare September 25, 2025 23:22
@TheEvilSkeleton TheEvilSkeleton changed the title doc: Expose all public headers portal: Include settings.h header Sep 25, 2025
@TheEvilSkeleton
Copy link
Contributor Author

What do we do with failed CIs?

@smcv
Copy link
Contributor

smcv commented Sep 26, 2025

What do we do with failed CIs?

In this case? Fix the bug and try again.

The build failure seems to be that settings.h is not C++-compatible (thus breaking the build of the Qt test app, because Qt is C++), because it uses the C++ keyword namespace as a function parameter name. That might be why settings.h was excluded from portal.h in the first place?

Renaming the parameter name, perhaps to ns or namespace_, seems like an appropriate fix.

@TheEvilSkeleton TheEvilSkeleton force-pushed the expose-all-headers-in-docs branch from 6ef48ed to 0e7edea Compare October 1, 2025 00:51
`namespace` is reserved in C++, which causes it to break
the Qt test app. This will be useful in the next commit,
which will expose `settings.h`.
@TheEvilSkeleton TheEvilSkeleton force-pushed the expose-all-headers-in-docs branch from 0e7edea to ae033d8 Compare October 8, 2025 12:37
@smcv smcv merged commit 437c3c9 into flatpak:main Oct 8, 2025
15 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.

4 participants