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.

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

Drop the unnecessary indirection of mc::BufferStreamFactory #3368

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RAOF
Copy link
Contributor

@RAOF RAOF commented May 2, 2024

We only create a mc::Stream in like 1 place, the implementation of BufferStreamFactory is trivial, and I'm about to remove all the parameters from the mc::Stream constructor anyway.

There's no need to keep this indirection (and the tests that check that we correctly call through that indirection)

@RAOF RAOF requested a review from a team as a code owner May 2, 2024 08:25
We only create a `mc::Stream` in like 1 place, the implementation of
`BufferStreamFactory` is trivial, and I'm about to remove *all* the
parameters from the `mc::Stream` constructor anyway.

There's no need to keep this indirection (and the tests that check that
we correctly call through that indirection)
@RAOF RAOF force-pushed the drop-buffer-stream-factory branch from 12d0d62 to 7789335 Compare May 2, 2024 08:29
Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.48%. Comparing base (4128d65) to head (99dda3f).
Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3368      +/-   ##
==========================================
- Coverage   77.52%   77.48%   -0.05%     
==========================================
  Files        1065     1061       -4     
  Lines       67869    67813      -56     
==========================================
- Hits        52618    52544      -74     
- Misses      15251    15269      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mattkae mattkae left a comment

Choose a reason for hiding this comment

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

A single question!

@@ -33,16 +32,6 @@ namespace mc = mir::compositor;
namespace ms = mir::scene;
namespace mg = mir::graphics;

std::shared_ptr<ms::BufferStreamFactory>
mir::DefaultServerConfiguration::the_buffer_stream_factory()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do anything in the symbols.map file to remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉

Hurray for extra CI checks!

@mattkae
Copy link
Contributor

mattkae commented May 3, 2024

Eh, it still seems to want the_buffer_stream_factory? I can check that out today

@RAOF
Copy link
Contributor Author

RAOF commented May 6, 2024

Huh. Thanks, symbol checks, for picking that up.

Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

I'm not against this, but with the_buffer_stream_factory() we had a customisation point.

I have no recollection of why that existed (apparently it was part of a refactoring I did 11 years ago), but are we sure hypothetical downstreams (or ourselves) we don't need it?

@RAOF
Copy link
Contributor Author

RAOF commented May 8, 2024

I'm not against this, but with the_buffer_stream_factory() we had a customisation point.

I have no recollection of why that existed (apparently it was part of a refactoring I did 11 years ago), but are we sure hypothetical downstreams (or ourselves) we don't need it?

I think it was a configuration point for testing purposes? Our BufferStreams used to have actual logic in them. I'm not aware of any downstream that currently overrides the_buffer_stream_factory() (QtMir definitely doesn't), nor can I think offhand of any time a downstream would want a way to wrap all the BufferStreams?

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.

None yet

3 participants