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

[x11-platform] Make window title a configuration option #2349

Merged
merged 14 commits into from Mar 3, 2022

Conversation

graysonguarino
Copy link
Contributor

@graysonguarino graysonguarino commented Mar 1, 2022

Fixes #2219 by making the initialized title an argument in X11Window, and if the argument is an empty string, sets it to the previous default, "Mir on X".

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.

This solves issue #2219

No it doesn't. (Yet.)

It is a good step along the way, but to solve the issue, there also needs to be a way to configure the title at runtime. We work with feature branches rather than continuously integrating because we're working across timezones and reviewing asynchronously (and not pairing).

Can you:

  1. add a further commit to this PR that sets the title based on a configuration option?
  2. Change the wording above to "Fixes: 2219" (that way github links the issue to the merge)

src/platforms/x11/graphics/display.cpp Outdated Show resolved Hide resolved
@graysonguarino graysonguarino changed the title Change window title from initialized to parameter Fixes: 2219 Mar 1, 2022
@AlanGriffiths AlanGriffiths changed the title Fixes: 2219 [x11-platform] Make window title a configuration option Mar 2, 2022
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.

All the issues I see arise from trying to treat "Title" as reference type (referring to a particular, shared string).

"Title" is a conceptually a value and best treated as such (the code will be simpler, more efficient and avoid the bug noted).

src/platforms/x11/graphics/display.cpp Outdated Show resolved Hide resolved
src/platforms/x11/graphics/display.cpp Outdated Show resolved Hide resolved
src/platforms/x11/graphics/graphics.cpp Outdated Show resolved Hide resolved
src/platforms/x11/graphics/platform.cpp Outdated Show resolved Hide resolved
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.

Some very minor tweaks for good style, it seems to work.

tests/unit-tests/platforms/x11/test_display.cpp Outdated Show resolved Hide resolved
src/platforms/x11/graphics/display.cpp Outdated Show resolved Hide resolved
src/platforms/x11/graphics/platform.cpp Outdated Show resolved Hide resolved
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.

Did I forget these last time?

src/platforms/x11/graphics/display.h Outdated Show resolved Hide resolved
tests/unit-tests/platforms/x11/test_platform.cpp Outdated Show resolved Hide resolved
tests/unit-tests/platforms/x11/test_platform.cpp Outdated Show resolved Hide resolved
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.

LGTM

bors merge

@bors bors bot merged commit fa0842c into main Mar 3, 2022
@bors bors bot deleted the rename-window-header branch March 3, 2022 09:50
@Saviq Saviq mentioned this pull request May 12, 2022
bors bot added a commit that referenced this pull request May 24, 2022
2423: Release 2.8.0 r=AlanGriffiths,graysonguarino,RAOF,wmww,Saviq a=Saviq

---
> - ABI summary:
>   - miral ABI unchanged at 4
>   - mircommon ABI bumped to 9
>   - mircookie ABI unchanged at 2
>   - mircore ABI unchanged at 1
>   - miroil ABI unchanged at 1
>   - mirplatform ABI unchanged at 23
>   - mirserver ABI bumped to 58
>   - mirwayland ABI unchanged at 3
>   - mirplatformgraphics ABI bumped to 20
>   - mirinputplatform ABI unchanged at 8
> - Enhancements:
>   - Move generated protocol code to build directory (#2300)
>   - Allow --app-env-amend to be supplied multiple times (#2333)
>   - Make window title a configuration option (#2349)
>   - Add fatal_error if unable to bind Wayland socket (#2350)
>   - Add `mold` to the list of supported linkers (#2353)
>   - Platform refactoring towards hybrid GPU support (#2358, #2378, #2407)
>   - Implement wlr_screencopy_unstable_v1 for screenshots (#2383)
>   - Refactor out mf::MirDisplay (#2406)
> - Bugs fixed:
>   - Synchronize buffer swaps to video frame in egl spinner (Fixes #2154)
>   - Do not give menus keyboard focus (Fixes #2324)
>   - Refactor Wayland keyboard input (Fixes #2331)
>   - Further simplify and correct keyboard focus setting (Fixes #2338)
>   - wl_pointer: do not send events when not compatible (Fixes #2341)
>   - Kill clients with error instead of sending unsupported (Fixes #2343)
>   - Initialize sig_handler_desc.sa_mask (#2386)
>   - Fix ThreadedDispatcherSignalTest.keeps_dispatching... (Fixes #2377)

Co-authored-by: Michał Sawicz <michal@sawicz.net>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
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.

Rename "Mir on X" window header
2 participants