Various improvements #29
Conversation
- Use different color when a widget is Enabled or Disabled - Fixed: bug with button widgets triggering by mouse events even if mouse wasn't on top of them - Fixed: some widgets didn't have enough spacing or had hardcoded values - Fixed: Listbox widget would iterate over all elements, even if they were not visible on screen. Now using CurrentClipArea instead.
- Added missing opengl3 items from guichan - Updated VS solution/projects to use vcpkg instead of hardcoded paths - Fixed compilation on Windows with VS 2022 - Included required dependencies (GL and GLM)
The loaded image was never released
This allows building with CMake or a normal Makefile
…d of a hard coded value
Add 2 pixels height to the height, which looks better
- Set Selected and Clear Selected - Exposed isDroppedDown, dropDown and foldUp
Add add_elements and clear_elements virtual functions
| } | ||
|
|
||
| return image; | ||
| return mImage; |
There was a problem hiding this comment.
Purpose of ImageLoader is to load several images, Here you need one imageLoader by image...
For RAII, it should return a smart pointer instead...
Unsure about minimal standard wanted for guisan though...
There was a problem hiding this comment.
I made these changes to ImageLoader to fix a memory leak problem I noticed (with Valgrind). The images allocated were never freed again, and if you loaded several in your application, the lost memory increased. Perhaps there's a better way of doing it, but this worked for me at least :) Open to suggestions.
There was a problem hiding this comment.
As I said, the proper fix would be to return std::unique_ptr<gcn::Image>, but it requires C++11 (and changes API).
Guisan has 2 usages of C++11 auto, so minimum standard version might be C++11... (but there is still API changes...)
Internal usage of Image::load (Icon, ImageButton, ImageFont) does the deletion.
Examples misuse it though.
Your changes would produce double free (unless you also modify those classes).
There was a problem hiding this comment.
Sorry for the delay, I'm having some health issues that make it very painful to use a computer the last few weeks.
I've added a free function as well, which can be used to delete the image when you're done with it. I call that from my code, when cleaning up. The examples here haven't been updated, as that was not in my scope during this PR (though I could modify them accordingly as well).
I don't see how this approach would lead to double free errors - indeed, I guess I would have run into them in my app, if that was the case. Perhaps I'm missing something you're seeing, but this approach seems to work fine for my needs. Could you elaborate more, please?
There was a problem hiding this comment.
as that was not in my scope during this PR (though I could modify them accordingly as well).
As I said, split that PR to be manageable, instead of continuing to make the PR huge. ;-)
Note that I am not a maintainer, but a contributor like you :).
I don't see how this approach would lead to double free errors
If you look at Icon:
Icon::Icon(const std::string& filename)
{
mImage = Image::load(filename);
mInternalImage = true;
setHeight(mImage->getHeight());
setWidth(mImage->getWidth());
}
Icon::~Icon()
{
if (mInternalImage)
{
delete mImage;
}
}
It does the deletion.
ImageLoader is a factory, so, if you want auto deletion, you would need a vector of all images created (or returning a smart pointer) and so fix usages which does the deletion.
|
I'll revisit this in a new PR eventually, closing this to avoid confusion |
I've been maintaining my own fork of this project for a few years now, and I've made various changes and improvements to make it better. I thought it's time to contribute those back upstream, so here you go :)
The bulk of them are minor things, to improve readability and reduce compiler warnings (things like type casting). Since they affect almost all the files, it's unfortunately a big PR.
There are a few improvements in some widgets, as well, that I needed for my project or that I found they made things better (e.g. some heights had hardcoded values, labels needed 2 pixels more, etc).
Finally, I've made some changes to the VS project: it now uses
vcpkgfor the SDL2 paths, so they don't need to be hardcoded anymore.And I've also added a Makefile and a CMakeLists.txt file, so you can use those methods to build the library as well, besides the existing ones.