[XLib, SDL] Add function window_get_icon(), window_set_icon(), etc.

Reporter: time-killer-games  |  Status: open  |  Last Modified: November 13, 2019, 03:32:58 PM

Well, for the first time in the roughly 10 years the ENIGMA game engine has been around, you can now natively set a PNG icon for your game that will show in the title bar, task manager, system monitor, and Alt+Tab process swapper. No thanks to those fat lards Josh, Robert, and fundies. No offense, I love you guys, but I couldn't take it any longer so I decided to step in and add Linux icon support.

Includes the following functions:

  • window_get_icon_index() returns the sprite index of the current icon

  • window_get_icon_subimg() returns the sprite subimg of the current icon

  • void window_set_icon(ind, subimg) sets the icon to use for the game from the given sprite resource

Screenshot_20190830_131818

As a small side note - this pull request also allows capturing SDL's window handle on linux for widget use. Although I still haven't fixed the SDL bug where the game doesn't respond and then segfaults, if you click the close button in the main game window's title bar over and over while a dialog is open. Not many people will do that so it's not a huge bug.

codecov[bot]  
>Codecov Report

Merging #1854 into master will decrease coverage by 0.07%.
The diff coverage is 5.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1854      +/-   ##
==========================================
- Coverage   25.15%   25.07%   -0.08%     
==========================================
  Files         177      178       +1     
  Lines       16844    16894      +50     
==========================================
  Hits         4237     4237              
- Misses      12607    12657      +50
Impacted Files Coverage Δ
ENIGMAsystem/SHELL/Platforms/General/PFwindow.h 100% <ø> (ø) ⬆️
...HELL/Universal_System/Resources/sprites_internal.h 100% <ø> (ø) ⬆️
ENIGMAsystem/SHELL/Platforms/xlib/XLIBwindow.cpp 49.75% <0%> (-2.69%) ⬇️
ENIGMAsystem/SHELL/Platforms/xlib/XLIBicon.cpp 0% <0%> (ø)
.../SHELL/Universal_System/Resources/spritestruct.cpp 74.18% <100%> (ø) ⬆️
ENIGMAsystem/SHELL/Platforms/General/PFwindow.cpp 36.5% <100%> (-0.34%) ⬇️
ENIGMAsystem/SHELL/Universal_System/roomsystem.cpp 30.58% <0%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 130e673...9962b88. Read the comment docs.

time-killer-games  

@JoshDreamland please review kthx
time-killer-games  

@RobertBColton Josh wants to know if you are ok with relicensing your libpng code you wrote under whatever SDL2 uses I think it's MIT or zlib. I want to translate this pr into a contribution to the SDL project itself, because they currently do not have a way to set an x11 icon for Linux games, and I am using both my and your code to do this, but mostly yours.
time-killer-games  

@JoshDreamland ready for review, your royal heinieness
JoshDreamland  

I thought you were going to remove the SDL integrations from Xlib?
time-killer-games  

I thought you were going to remove the SDL integrations from Xlib?

I kinda did. What you're looking at is widget related stuff, which i thought you would be ok with, because you had previously merged X11 code used in my widgets for checking the KWIN_RUNNING Atom, which would be used regardless of whether the user chose X11 or SDL.

If you want me to change it again, that's fine, but it's removing more code I took the time to write (although I understand it isn't much code or time that was spent on this to begin with, the time does add up when I also have to wait for you to review).

JoshDreamland  

I was mostly just confused about what's going on. The flow of code around window_id_init is difficult to follow, largely because it introduces a new constraint that some platforms (X11, SDL) must adhere to, but you didn't document what those constraints are, or what platforms they actually apply to.

In general, we avoid creating these couplings between systems—it leads to spaghetti code. Particularly with one or more of these systems missing from CI coverage, it's likely to cause breakages, as people won't realize that renaming a method in one system will break another. In general, healthy code means contributors can be confident that changes they make will not introduce unintended side-effects. So the less intimacy you have between systems, the better.

There are, of course, places in the engine where systems require functionality from one another. When we need all windowing systems to support a particular routine, we add it to platforms_mandatory. There are functions in that header that are similar to what you seem to want. For instance, initGameWindow is probably the place where you would have SDL stash its information somewhere. Between initGameWindow, initInput, and EnableDrawing, I suspect whatever opportunity you need to to take is covered by one of those methods.

There are other places in the engine where you'll see this kind of intimacy. The biggest are in the "Bridges" and "General" folders. Bridges depend directly on two systems, and access members between each directly to tie things together. Bridges are where you'll find code for binding libraries, like WGL and GLX, which depend on the HWND/X resource and the GL context.

We don't currently have a bridge structure to bind widget systems to window systems ("Platforms"). I am certainly not opposed to adding that bridge system, but we should do so in a way that is analogous to existing bridges, rather than tying code together between two systems.

time-killer-games  

@JoshDreamland so then, heinie, what might you suggest I do? (Also, do you mind if I call you heinie for now on?)
RobertBColton  

Yeah you can relicense my code, but not remove my name, and send the fixes upstream to SDL where they belong.
time-killer-games  

Yeah you can relicense my code, but not remove my name, and send the fixes upstream to SDL where they belong.

I wasn't planning on removing your name, this is your code mostly and that would be a rather big asshole move on my part if I did that. Thank you for the permission, that is great news!

JoshDreamland  

Is there a reason you can't move that code into the existing initGameWindow routine? Or else, invoke that routine from inside that one (so the naming convention is contained within SDL).
time-killer-games  

@JoshDreamland No, I can't just stash platform specific code in initGameWindow in SDL, it relies on X11. I set it up the way it is so that the Makefiles would ensure all that Linux-specific code doesn't attempt to be compiled anywhere but on Linux. If I put that stuff in initGameWindow, Windows and Mac wouldn't build anymore, unless I did platform macros in the code itself and not the Makefiles which I thought you guys were completely against doing.

(Yes, I'm aware Mac doesn't build regardless, but this would just further complicate that issue).

JoshDreamland  

As I said, feel free to define a helper method inside of SDL, call it from initGameWindow, and implement it differently between SDL/Win32 and SDL/X11. The distinction is that other window systems (like Xlib) don't have to worry about constraints within SDL.
time-killer-games  

@JoshDreamland ready for review
time-killer-games  

@JoshDreamland before this gets merged and you accuse me of breaking SDL. SDL has been unusable for me for a long time now, dating farther back than any of my recent pr's, including the windowing related ones obviously because I didn't touch SDL's code in those pr's at all. On Windows, the SDL window is always a blank black window for me. On Linux, I get a window client area that looks like the grey fizz on an old TV screen. Just wanted to point out these issues before this gets merged and you blame me for something I didnt break. I just never cared to mention these bugs with SDL until it became a concern that I might get blamed for it when it wasn't me who caused it. Before this pr, I haven't touched SDL hardly at all other than for implemented joysticks half way. No one blame me. You can test the current enigma-dev master to see and verify this pr doesn't cause it. I'm the only one who has been actively contributing as of late so I don't want that to be a blind excuse for any false accusations. SDL should be trashed honestly. Here's what happens on Linux when you drag a window over SDL's window:

Screenshot_20191014_002525

As you can see, SDL is already broken and can't be used, and it has been that way for a while. I find refusing to merge this pull request which has changes made that are technically correct a bad idea. I don't intend to fix things that I don't know how to fix in this pr that were already broken; it would be in your best interest to go ahead and merge this when the CI passes this time. Progress is progress, Refusing to merge this for reasons that don't make sense is counter productive and not progress.

At least from my testing on Windows just now I was able to verify Direct Sound and SDL both work together just fine with my code changes. No surprise. It's the graphics that are broken, which I am not responsible for. No surprise. (NONE of the graphics systems work with SDL btw, not just D3D).

I don't understand why we have SDL now that fundies is not helping. This means Mac and Android will never get finished, and I thought these two platforms were the only reason we ever supported SDL.

time-killer-games  

@RobertBColton it turns out me and Josh were wrong, SDL does in fact have icon support for linux already. Does this mean we should set the license back to GPL? I was going to create a GameMaker extension which uses the code and I shared it on the GameMaker Community in my code snippet topic - https://forum.yoyogames.com/index.php?threads/code-bank-share-useful-code.60575/#post-406540 - Please let me know if you want me to take this down, since the original reason we relicensed it under zlib has been nullified.
RobertBColton  

It's ok, I don't mind.
time-killer-games  

It's ok, I don't mind.

Thank you!

Please sign in to post comments, or you can view this issue on GitHub.