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

Update window title with (on <client_machine>) if client is remote #790

Open
wants to merge 2 commits into
base: icewm-1-4-BRANCH
Choose a base branch
from

Conversation

flipbit03
Copy link

@flipbit03 flipbit03 commented Dec 4, 2024

This pull request introduces changes to display the client machine's hostname in the window title if it differs from the local machine's hostname. The most important changes include adding methods to fetch the client machine's hostname and updating the window title rendering logic.

Sample screenshot of Xephyr with a remote Google Chrome:
image

Enhancements to window title display:

  • src/wmtitle.cc: Updated YFrameTitleBar::paint method to append the client machine's hostname to the window title if it is different from the local machine's hostname.

New methods for fetching client machine information:

  • src/ywindow.cc: Added YWindow::fetchClientMachine method to retrieve the client machine's hostname using XGetTextProperty.
  • src/ywindow.h: Declared the new fetchClientMachine method in the YWindow class.

Local hostname retrieval:

  • src/yxapp.h: Included <unistd.h> for gethostname function and added localHostname member and getLocalHostname method to YXApplication class to retrieve and cache the local machine's hostname. [1] [2]

@flipbit03 flipbit03 changed the title Update title if client is remote Update window title with (on <WM_CLIENT_MACHINE>) if the client is remote Dec 4, 2024
@flipbit03 flipbit03 changed the title Update window title with (on <WM_CLIENT_MACHINE>) if the client is remote Update window title with (on <WM_CLIENT_MACHINE>) if client is remote Dec 4, 2024
@flipbit03 flipbit03 changed the title Update window title with (on <WM_CLIENT_MACHINE>) if client is remote Update window title with (on <client_machine>) if client is remote Dec 4, 2024
@gijsbers gijsbers self-assigned this Dec 5, 2024
@gijsbers gijsbers added the enhancement The report requests an enhancement beyond the intended behaviour. label Dec 5, 2024
@gijsbers
Copy link
Collaborator

gijsbers commented Dec 5, 2024

First impressions:

  1. Does every user of icewm with remote applications want this feature?
  2. In icewm code we prefer to use YTextProperty over XGetTextProperty.
  3. The process of fetching the client hostname repeats on every paint.
  4. In the client code it is customary to check properties on initialization.
  5. There is no support for internationalization. Neither for right-to-left languages.

@flipbit03
Copy link
Author

flipbit03 commented Dec 5, 2024

Thank you for the review, @gijsbers!

Does every user of icewm with remote applications want this feature?

I'm honestly not sure - I am a long time XFCE User and they have this as a blessed default feature, which was very useful for me to not get crazy over multiple windows from a variety of remove servers, so I figured I'd put out the simpler version first (where it's just on) and gather feedback. The point is - I didn't even know I wanted such a feature because it had never crossed my mind (that such capability was possible or that we had this metadata coming in from a remote window), I was just exposed to it via XFwm4, and it was very helpful.

If you feel strongly, we can definitely implement it as a config feature - I'd appreciate any pointers on that.

In icewm code we prefer to use YTextProperty over XGetTextProperty.

I'll look into using this specific function, if that's prefferable.

The process of fetching the client hostname repeats on every paint.
In the client code it is customary to check properties on initialization.

True! This is not ideal and adds additional protocol overhead. What do you think about fetching this property when the window is created, and then caching it in the instance, just like I did with localHostname?

There is no support for internationalization. Neither for right-to-left languages.

Yup, we need to improve that. Can you give me pointers or examples in the codebase that I could use as a starting point in my research?

I'd like to point out that the feature in XFwm4 also lacks internationalization, and while this is a clear shortcoming that should be fixed, I feel that the feature enough brings so much value that I would not block it's implementation just on this specific basis (Working on a daily basis with remote windows is less common, that means any improvement for that specific, more niched user base is a boon). (I'm happy to agree to disagree and discuss - I just really like IceWM, I've been a user for 20+ years, and want to improve the tool as much as I'm able to.

@gijsbers
Copy link
Collaborator

gijsbers commented Dec 5, 2024

Study getNetWMPid, rightToLeft and fPid. You can't just copy XFCE features by injecting a few lines, if you want it to be acceptable. It has to be maintainable over the long run with little cost.

@flipbit03
Copy link
Author

flipbit03 commented Dec 5, 2024

I'll look into the implementation for the RTL/internationalization stuff, which is clearly needed. Besides that, I'd appreciate a little more clarity on what specific bits, or techniques used, can be improved to attain what you consider as "maintainable over the long run with little cost". These statements, as they are put, are not actionable on my end without some additional context.

I'm still waiting on your opinions for the questions further up, regarding on whether you feel this shouldn't be a "default on" feature, etc. Thanks!

@gijsbers
Copy link
Collaborator

This is a tiny feature. Therefore any burden due to: 1. difficult to comprehend, 2. has unique implemention techniques unlike similar features, 3. has the risk of generating issues by users, 4. has the risk of causing bugs which need to be fixed urgently in a new release, are to be avoided.

"Default on" I'd say no for now, so as not to disturb anyone unexpectedly, causing issues, which are to be avoided.

@flipbit03
Copy link
Author

@gijsbers agreed. I'll work on making the change modular/togglable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The report requests an enhancement beyond the intended behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants