Small screen position feature

[DavidRGriswold]

trafficstars

This will address #332. Currently in draft. Steps for completion are:

• [x] Update android to use Large Screen instead of the MobileLandscape layout (separate #PR is in for this already), this branch builds off of that one and will be updated to incorporate any changes in that PR

• [x] Enable the feature internally by editing FrameBuffer Layout code to support a SmallScreenPosition enum. Choices are TopRight, MiddleRight, BottomRight, TopLeft, MiddleLeft, BottomLeft, Above, and Below. Above and Below are centered.

• [x] Add settings to the INI file and all the read setting / write setting code to enable this as a config file setting.

• [x] Add desktop UI for user to edit this setting

• [x] Add android model and UI to edit the feature

  • [x] At the same time, the Large Screen Proportion setting should also be exposed on android

Bugfixes once mostly complete

• [x] Fix the weird issue with entering floats in the slider for large screen proportion where it doesn't let "3." be the text so you end up having to do something weird to get 3.2

• [x] What the heck is happening with AppImage and why?

[DavidRGriswold]

This is what the desktop GUI for this looks like right now. It might benefit from some grouping or better labeling, design isn't my strong suit.

[DavidRGriswold]

Most recent build adds a submenu that is working (it was so much harder than adding a combobox in settings!)

The only thing I wish I could figure out but can't quite is how to disable the actual menu item, rather than just the items inside, when other layouts are chosen. This is what I have:

I'd prefer for the menu item itself to be gray and not be able to open the submenu, but calling setEnable(false) on the menu didn't seem to do that. Maybe I need to be wrapping an action somewhere? I don't know, QT is still something of a mystery to me.

[DavidRGriswold]

I am not sure what is causing this to crash on trying to build the appimage. It looks like "Above", "Below", and "None" are all being interpreted as macros from X.h or something, which is causing issues when those values are being used as enum IDs. I could change the Above and Below enum ids, but it's also causing an error in one of the externals so I'm wondering if something else is going on.

[DavidRGriswold]

I'm marking this ready for review to potentially get some more eyes on it since I think it is almost all the way done. Two bugfixes are listed in the initial comment still to deal with - I think I have a pretty good idea on how to tackle the first one when I get a moment (and it is an annoyance, not a killer) but if anybody wants to help me unravel the weird mystery of why this is failing to compile on linux I'd be appreciative.

For now, the android UI only has these settings inside the main Settings menu, not the popup menu. Adding it to the popup menu is a pretty big job so I would rather get the current version looked at, reviewed, and approved, and then I can add that as another PR at a later date.

[BlurrySquire]

I'm on my phone so I am unable to check the GitHub ci. It says the checks are in progress but when I click on them it says they need approval to run. Unsure where I give approval on phone.

Would you mind checking the build section of the Linux app image ci job and seeing if there is an error reported?

[DavidRGriswold]

I'm on my phone so I am unable to check the GitHub ci. It says the checks are in progress but when I click on them it says they need approval to run. Unsure where I give approval on phone.

Would you mind checking the build section of the Linux app image ci job and seeing if there is an error reported?

Here is the error

It used to also complain about enum values named "Above" and "Below" but I changed those so now that complaint is gone. I didn't put the value "None = 0" in there, though, so I'm really unclear on why this is failing on this branch but not all the others that have that line. It seems like it's trying to expand "None" as a macro (incorrectly obviously) instead of seeing it as a simple enum identifier, but I have no idea what I changed to make that happen. Could it just be something I screwed up with the externals somehow?

[BlurrySquire]

I don't think it is the externals. I would double check cam_params.h as it is complaining that the enum value None is a macro in an x11 header. I know you said you removed the enum value but the error is reporting that it is still there from what I can tell.

Macros are stupid I had one issue before with the windows API defining a macro that I used as a function name.

There is actually a kinda hacky solution. #undef None. I had to use this with my windows API macro issue.

Edit: deleted my original message because it was sent twice for some reason.

[DavidRGriswold]

I don't think it is the externals. I would double check cam_params.h as it is complaining that the enum value None is a macro in an x11 header. I know you said you removed the enum value but the error is reporting that it is still there from what I can tell.

Macros are stupid I had one issue before with the windows API defining a macro that I used as a function name.

There is actually a kinda hacky solution. #undef None. I had to use this with my windows API macro issue.

Edit: deleted my original message because it was sent twice for some reason.

I'm sure I could fix it that way, but also I did not touch cam_params.h at all, so I guess I am just wondering why this branch is getting that issue but none of the other branches are. Maybe a merge from master would magically fix it?

[rtiangha]

Skimming through things, why I think it's happening in the AppImage build and nowhere else off the top of my head:

• Linux is the only OS that uses X.org (obviously)
• None is a reserved name in X.org (so people shouldn't use that as a label in their code).
• When you added #include <common/settings.h> to src/core/frontend/framebuffer_layout.h, settings.h pulls in cam_params.h which is why this error gets triggered now (as cam_params.h didn't normally interact with X.org while framebuffer_layout.h does hence never encountering the issue before this PR).

I haven't tried it yet myself, but the easy answer would be to modify cam_params.h and rename its None (and anywhere else it's used) to something else (maybe Disabled based on the context? Don't ask me; I suck at naming things).

Edit: Looks like None is used for Flip and Effect when it comes to the camera, so let's say we rename it to Disabled instead; these would be all the files that would need to be modified:

[reg@PROBOOK430-G4 Lime3DS]$ grep -r "Effect::None" src
src/core/hle/service/cam/cam.cpp:            context.effect = Effect::None;
src/core/hle/service/cam/cam.h:        Effect effect{Effect::None};
src/lime_qt/camera/qt_camera_base.cpp:    if (effect != Service::CAM::Effect::None) {
src/lime_qt/configuration/configure_camera.cpp:    previewing_camera->SetEffect(Service::CAM::Effect::None);
[reg@PROBOOK430-G4 Lime3DS]$ grep -r "Flip::None" src
src/core/hle/service/cam/cam.cpp:            context.flip = camera_id == 1 ? Flip::Horizontal : Flip::None;
src/core/hle/service/cam/cam.h:        Flip flip{Flip::None};
src/lime_qt/configuration/configure_camera.cpp:    previewing_camera->SetFlip(Service::CAM::Flip::None);

plus cam_params.h. Of course, the camera would need to be re-tested to ensure no regressions; as an aside, I don't normally use the camera functionality so I have no idea where one would set 'Sepia' as one of the camera options, but it's listed in cam_params.h.

Edit: Nope. Just tried replacing the None references in cam_params.h, but it's now complaining about the ones in settings.h too (which implies everything that settings.h touches would need to be changed too). Is there a way to rewrite things so that framebuffer_layout.h doesn't need to be aware of the Settings namespace? As in store the Settings value in a temporary variable first before making the call and pass those along, and redefine the framebuffer_layout.h functions to take in regular ints rather than Settings::XXX instead?

[DavidRGriswold]

Am I really the one that added the Settings namespace call here? I guess so, because previously all the settings calls were handled in emu_window.cpp. I guess we could have emu_window.cpp pass in the integer value of the setting as an argument to the large screen position call, and then translate that in framebuffer.cpp directly, but that seems rather obnoxious and would mean if we ever switch the order or anything of that enum it might have a surprising effect. Though I guess that isn't too likely.

Could we use the #undef None thing mentioned by BlurrySquire inside of settings.h and cam_params.h to solve this so it doesn't try to get translated as a macro?

[BlurrySquire]

I don't recommend that solution I mentioned. It is hacky and a last ditch effort kind of solution.

[rtiangha]

Actually it's not that bad. I can't work on this now as I have to go, but high level, just pass Settings::XXX.getValue() (which I think is an int) to the framebuffer functions, and change the function to accept ints rather than Settings::XXX. Everything else can just stay the same; you can even keep the same variable names.

[DavidRGriswold]

I don't recommend that solution I mentioned. It is hacky and a last ditch effort kind of solution.

Eh, I think having macros that replace a perfectly good identifier across everything touching a file is already pretty hacky, so if we need to be hacky to get around X.org being hacky, so be it.

[DavidRGriswold]

Actually it's not that bad. I can't work on this now as I have to go, but high level, just pass Settings::XXX.getValue() (which I think is an int) to the framebuffer functions, and change the function to accept ints rather than Settings::XXX. Everything else can just stay the same; you can even keep the same variable names.

Yeah this will work, I just is annoying. The whole point of enuns is to get meaningful names instead of ints and also to add flexibility if we ever want to change the order around. Using the ints directly is brittle.

[rtiangha]

Could also move the enum definitions to a different .h and import that into settings.h and framebuffer_layout.h. Like maybe to src/common/common_types.h or something similar in the common directory (or make a new one like frame_utils.h or something).

[DavidRGriswold]

Could also move the enum definitions to a different .h and import that into settings.h and framebuffer_layout.h. Like maybe to src/common/common_types.h or something similar in the common directory (or make a new one like frame_utils.h or something).

I like this solution best I think

[DavidRGriswold]

Okay, so we seem to have four possible solutions to this annoying linux compilation issue, all with upsides and downsides. I'm going to list them here and then maybe we can get @OpenSauce04 to weigh in.

1. Rename all uses of the word None in both cam_params.h and settings.h to something else, and adjust those everywhere needed.

   • Disadvantages: It would require changing lots of files that refer to those two enum values (though not really that many individual lines of code I think). Probably best done as a separate PR then merged into this. Also means we are bowing to X.org's annoying choice to use macros, but that's just my personal annoyance.
   • Advantages: it means this wouldn't come up again the next time we might need to import settings.h into a file that interacts with x11, can keep my actual code for this feature as written with the new setting defined in the same place as all other settings and used where needed.
   • Update after testing: This only required changing 14 lines of code in the end - neither of these specific Enum values are used in very many places. Build result is here, all seems fine.

2. Move the SmallScreenPosition enum into a different file and namespace. Import that new file into the places that use it, including settings.h itself, and do not import settings.h into framebuffer_layout.

   • Disadvantages: It's weird to have just one setting definition in a different file from all of the others. If we ever need to import settings.h into an x11-touching file again, this will happen again.
   • Advantages: relatively clean and easy to do, only requires editing code already in this PR, no prior code.

3. Remove settings.h import from framebuffer_layout.cpp, pass the value of the setting when largescreenframelayout() is called as an integer, and use the integer value directly (or a local enum) to change the behavior rather than the setting itself.

   • Disadvantages: brittle. If we need or want to change the order of the enum at a future time, we would need to change this file that no longer obviously connects since it doesn't show up as using that enum.
   • Advantages: easy to do, no new files, settings gets to stay in settings.h.

4. Use #undef to disable the macro in settings.h and cam_params.h so that the preprocessor doesn't try to replace the perfectly good identifier None with something that doesn't make sense.

   • Disadvantages: hacky. I suppose could also cause issues if the macro needs to be used sometime after the #undef directive (though I imagine that is solvable with a little reading about c++ macros).
   • Advantages: Means we can keep using the perfectly good code that works on everything but linux without the disadvantages listed for other options above.
   • Update after testing: this one seems to work fine and only needed additional two lines of code, though I can't actually test the linux artifact myself since I don't have a graphical linux install currently. The lime-build for the branch with this change is here .

[rtiangha]

I prefer option 2 myself. And in terms of creating a new file (if none of the existing .h files is appropriate), even if it's just used for one purpose now, it could be used later on if new functionality is developed. And besides, there's now precedent for such a thing (ex. see #390 ).

Edit: Actually, looking closer at things, Option 1 isn't bad either. Not too many changes as I thought it would be, and it fixes the settings.h problem needing to be imported into video stuff in the future. I think I'm leaning towards this one now (especially since the code is now written), but I'm cool with either.

Edit 2: I'm changing my vote to Option 1. Upon reflection, I think it's the more future-proof choice because it eliminates the settings.h conflict with X.org which would never have been exposed if not for this PR; if not eliminated now, then someone else may inadvertently stumble upon the conflict again in the future. Plus, David already wrote the code on one of his branches so may as well use it.

[rtiangha]

@DavidRGriswold I was playing around with this PR on Android using Option 1 (Rename all uses of the word None in both cam_params.h and settings.h) and it seems to work well, but it introduced a bug where if you clear any slider text field in the program (i.e. delete all the characters), as soon as the field is blank, the program will crash.

I've submitted a PR against your PR branch that should make SettingsAdapter.kt a bit more resistant to null pointer exceptions; maybe I went overkill, but at least the bug no longer occurs as far as I can tell. If you merge it in, it should fix the problem (and make it pass the build checks too).

[DavidRGriswold]

@DavidRGriswold I was playing around with this PR on Android using Option 1 (Rename all uses of the word None in both cam_params.h and settings.h) and it seems to work well, but it introduced a bug where if you clear any slider text field in the program (i.e. delete all the characters), as soon as the field is blank, the program will crash.

I've submitted a PR against your PR branch that should make SettingsAdapter.kt a bit more resistant to null pointer exceptions; maybe I went overkill, but at least the bug no longer occurs as far as I can tell. If you merge it in, it should fix the problem (and make it pass the build checks too).

Hey thanks! I thought I had fixed that slider bug, but I guess not.

[DavidRGriswold]

At this point, I believe this feature fully works. I went with Option 1 for the Appimage issue, since it greatly reduces the chance of this bug being introduced again in the future. @OpenSauce04 , it's a big one so take your time, but I think this is ready for the review process.

[rtiangha]

I've been playing around with it for a bit, so for what it's worth, I approve it since the bug I found was fixed (the null pointer exception when emptying slider fields), but it could use some more functionality testing because I wasn't trying to see if anything was broken when I was playing around with it.

[DavidRGriswold]

Assuming this is likely to get reviewed and merged in the nearish future, I will code the requested screen gap feature against this code, since they clearly overlap significantly.

[DavidRGriswold]

Latest commit reduces codebase by replacing the DefaultLayout entirely with a call to the LargeFrameLayout setting the scale to 1.0 and choosing the "Below" setting for it. This also uncovered a bug, which I then fixed. I still need to re-test the "upright" mode.

[OpenSauce04]

I will provide a review for this code tomorrow evening

[OpenSauce04]

In terms of functionality this seems to work great on both desktop and mobile. I have not found any issues on that front.

[DavidRGriswold]

If you are trying to do a really full-full test, I would recommend also testing the resolution scale feature (which seems to be most obviously used in screenshots on desktop), test in Upright mode on desktop too, make sure to test swapped screens, and test with the scale factor = 1.00 (which caused a bug for a bit because it means the "small screen" might actually be bigger than the large screen when swapped.) I tried to test all of those but it is a lot of combinations!

[OpenSauce04]

...which caused a bug for a bit because it means the "small screen" might actually be bigger than the large screen when swapped.

I have found a combination of options that causes this issue and also seems to make the top screen become invisible:

• Enable "Rotate Upright"
• Enable "Swap Screens"
• Set Small Screen Position to "Above large screen" or "Below large screen"
• Set the Small Screen Proportion to 1.24 or less

[DavidRGriswold]

Hmm thought I fixed that but maybe it was on a different branch. I'll dig in this afternoon.

[DavidRGriswold]

Honestly the fact that upright mode is coded completely piecemeal is kind of crazy. I should really implement a rotateLayout method that just rotates the layout after creation.