-
-
Notifications
You must be signed in to change notification settings - Fork 960
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
feat(ui): list available displays with select
#2490
base: master
Are you sure you want to change the base?
Conversation
This looks like a nice addition. Thank you for the PR! |
This may cause conflicts with #2032 |
@Hazer Heyo, as RA mentioned it will conflict with my PR a little. Could I ask you to change this structure:
to
? Also, on Windows primary display is a display that has origin coordinate of (0, 0). There can be multiple primary displays. |
@ReenigneArcher That PR is amazing, maybe I'll try writing some of the macOS code needed in there, will that be a library with another repo or will be merged in the main code? |
@Hazer it will mostly be in a separate library... in order to not grow the Sunshine code by 20%. @FrogTheFrog is doing the heavy lifting. They are slowly porting things over. https://github.com/LizardByte/libdisplaydevice |
Sure, I was already discontent with the id as it was and wanted to change it, was currently reading #221 to gather ideas for it, string is the way to go.
As I was writing this PR, "primary display" became less and less something simply achievable, it seems if we don't use x11, any pure wayland compositor, has no specific API for that, and people relies on asking the DE (KDE, GDM, etc) directly, so it's not doable, I was already thinking of removing it, or making it has any display that has origin coordinate of (0, 0), but I was trying to find out if it made sense for linux. So meanwhile, you gave me the idea to rewrite it something like this: struct display_options_t {
std::string id;
std::string friendly_name;
current_display_settings_t current_settings;
};
struct current_display_settings_t {
display_device::display_origin_t origin; //< This doesn't exists yet, but could be a nice addition to the API
display_device::display_mode_t mode;
bool current_display_settings_t::is_primary() const {
return origin.x == 0 && origin.y == 0;
// or maybe even:
return origin.is_primary();
}
}; @FrogTheFrog what do you think? Meanwhile your PR is not merged, I just create a similar struct with the same name, we just replace it when it arrives. Also @FrogTheFrog, could your |
After some deliberation, I have decided not to expose anything more from the inner API (including types apart from the needed ones) than :
as we just don't need this information exposed to Sunshine at the moment (at least for Windows). Everything is done by passing the configuration anyway. So, the mode struct will not be available. I would recommend keeping the boolean (as you're now exposing Windows specific origin point logic to Sunshine) or just dropping it altogether as you've wanted.
HiDPI is a standalone setting hidden somewhere like the HDR and is not influenced by the display mode. That aside, please don't use the same namespace as I'm only working on Windows code and for other OS'es it will have |
@FrogTheFrog That kinda sucks for me, I was expecting to just import the
Actually, I don't know, I think it's not a good idea for other platforms as a boolean either, it seems wayland can use the same origin logic, but via offset, and macOS can use the same origin logic but will ever only have one (0, 0) that will always be the primary, it's not windows related, I'm just not sure about x11 yet, but probably yes or will need a origin coordinate conversion, but same stuff. Also, with more information, I can show this in the list for user selection, for example, if the monitor friendly name is something I don't recognize, or maybe like my linux sample with "VirtualX" all over the place, the user will be able to see the current resolution, something like:
And if I feel fancy, I could draw into Canvas all displays layed out using origin/resolution, so I'm not sure I want just the boolean anymore... But I'm still discovering what I'll really do. |
I would suggest to keep it simple for now (don't make the mistake I did...) and add new functionality little by little. My other problem is that I have too much code in the PR already and while I'm not against your suggestions, I don't want it to become a scope creep where I keep adding and adding stuff non-stop. I just want to merge my stuff already :'). That being said I could add current display mode to |
Actually please find out what data you really want and then let's discuss it. Also I think I've misunderstood it. If you want to reuse the "extended" (I really should go to sleep...) |
ec5f163
to
35cf375
Compare
New proof-of-concept, only working in macOS right now:
Screenshot |
Hope you got some rest, now I'm the one way past the bedtime |
I got slightly sick :D Got a few points/questions:
|
Stop commiting and go to sleep! |
@FrogTheFrog Hope your feeling better! I got some rest, and some food poisoning later on... So I got even more rest! Those are great points, that I'm not sure how to answer myself, and I probably should do a closer look into windows and your work so far, it will be easier if I understand from the same PoV, but at first glance: 1.Not sure, macOS handles as Double, Linux seems like a mess, x11 and wayland have different implementations, but it also seems to change between DEs, it seems Gnome uses Integer, while KDE uses Float and supports Fractional Scaling, while every other tool may have it's own standards, in my mind initially, double/float was fine. I think we can use the same pattern you're using with windows, but can you show me how to convert between 2.I see what you mean, but I'm not sure yet, let's take my MacBook screen for example, So when streaming, that information is useful, as I may want to stream in The only issue is when changing "resolutions" in macOS, at least before Sonoma, we are only changing the scale actually, the native pixel count will still be 2560x1600 when I'm rendering my screen in "1440x900" (it's called 'scaled looking like 1440x900' in the settings), it just changes the scale to Sooo, if we think in math terms it's represented as 3.ahhahaha yeah, I was sure I wasn't using the right values here, I just wanted to test it and be done with it for the day tbh, there's another APIs to get this value, so I wasn't even sure I should use this one. Actually, I need this info, given 60hz, which is my monitor refresh rate, what are the correct values for ==== I probably should try implementing the actual mode change for macOS as a PR for the libdisplay and actually map the available APIs for mode change in macOS, I already have some idea, it's mostly around the crusty PS: Sorry about this essay size |
I think I am (or we) approaching this from a wrong point. We don't need to expose the mode structure at all (I would prefer to keep it hidden at this point since it's very OS dependent). I still want to reuse the How about we have the following:
|
Splitting rendering data from config files data, we avoid a lot of potential mistakes and we can better evolve the API, information used only to display the UI components has no business being in the same object as actual config file properties
Mostly copied code from LizardByte#2032 to try a PoC for the API I want to use
Missing Windows and Linux implementations, but achieved an OK PoC for macOS, time to clean and do the basics for Windows
6cfb439
to
b03b874
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## nightly #2490 +/- ##
==========================================
+ Coverage 6.99% 8.18% +1.19%
==========================================
Files 87 49 -38
Lines 17679 8181 -9498
Branches 8399 3818 -4581
==========================================
- Hits 1237 670 -567
+ Misses 13745 7125 -6620
+ Partials 2697 386 -2311
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I'll split this PR next month or so, I think it will be better to postpone, now that I have a better understanding of the whole stack, and I have other improvements planned to be merged first. |
Heyo, I have reached a point where I am implementing the last function for the OS abstraction layer - the enumeration function. After thinking for a while, I would like to have the following structure in the end:
I have trimmed the fat and kept it simple (the denominator was a bad idea for the public interface...). Since it will now live in the library, I will no longer expose it as a JSON via As you can see, I am using the |
Sorry for seemingly out-of-nowhere comment struct Info {
Resolution m_resolution {}; /**< Resolution of an active device. */
float m_resolution_scale {}; /**< Resolution scaling of an active device. */
float m_refresh_rate {}; /**< Refresh rate of an active device. */
bool m_primary {}; /**< Indicates whether the device is a primary display. */
Point m_origin_point {}; /**< A starting point of the display. */
std::optional<HdrState> m_hdr_state {}; /**< HDR of an active device. */
}; @FrogTheFrog maybe use |
I thought about that initially, but I don't really need to use the |
FYI, I have already finalized the "public" types and am happy about them (for now): |
Wait, isn't this |
In my specific case, the denominator is always 10 to the power of For converting floating point back to rational, I luckily do not need to be too precise and just multiply floating point by 10000 and give Windows a rational of For converting rational to floating point, I just do the math. For persistently storing prev. settings, I just store the rational struct itself. |
In any case the user would input the refresh rate as a string/number (e.g. |
That's rather dangerous assumption, unless all APIs state that explicitly. And consequences are multiplied because you're relying on it in a public interface. |
Sorry, I'm bad at explaining stuff. I make no such assumptions. The types I have provided a link for are for humans/HMI (for the lack of better words). When I'm converting human input to the rational, the denominator will always be Internally I do not create new types and just use the Windows' ones. There are a few exceptions where I need to types to be saved to a file for persistence, so I've just made my own types (like The types that Hazer wants are for displaying info in the HMI only and would not be used for configuration anyways. |
That aside if you think that we should use rational type instead of float in general, just please say so. I have no preference and would gladly use it instead of floating point @ns6089 |
I think it's either you're overcomplicating things, or I'm oversimplifying them.
|
|
So, in short @ns6089. After thinking about it, I would like to use Would you still say that I should use something like:
|
Yes, I think this is the best. |
@FrogTheFrog sorry I missed the whole discussion, as it started while I was moving to live in another city and I got behind the amount of notifications I was receiving while doing so. The only thing I didn't quite like, but I still think it's a good decision, is hiding the I'm not a fan of Again, amazing work, and I'll be revisiting this very soon. Also, don't be afraid to ping me on Discord if I get AWOL or if you need faster input from me. I sent you a message and added you as friendo. |
Description
This is a PoC, and a proposal, for allowing the screen to be selected via a list, instead of having to search for an ID and write it down. Also, this PR doesn't cover it, but we could in the future, further improve it, and allow the client to request it and maybe remotely select the preferred display.
For this PR I took basically the same code from every
display_names()
and stripped it to just the basics, but keeping the check, fail-overs, and the same counting for IDs as the other methods use, so we have a 1:1 mapping. I took a bit more effort in some targets than others, but I focused in the targets I could build and run myself first.All targets are implemented, but some probably will need some more work done, testers are really welcome.
I don't consider this as a breaking change, because:
Depends on
Built and run so far
Screenshot
macOS
Windows
Linux AppImage x11
Type of Change
Checklist
Branch Updates
LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.