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

ChildWindow Buttons #61

Merged
merged 1 commit into from Jul 2, 2016
Merged

ChildWindow Buttons #61

merged 1 commit into from Jul 2, 2016

Conversation

@Ruckamongus
Copy link
Contributor

@Ruckamongus Ruckamongus commented Jun 28, 2016

The ChildWindow widget now allows any combination of minimized, maximized, and closed buttons - or none at all. Also, you can set the button text for each button from the default values of "x", "_", and "^" using the function setTitleButtonsText().

This is what it looks like:
image

As far as I can tell it plays nicely with the title bar alignments regardless of the number of buttons (proper text clipping and such). The theme file had to modified slightly to take advantage of the new buttons. I've attached my texture and theme file "Hedon.zip".
Hedon.zip

For testing convenience here is the contents of main.cpp I used:

#include <TGUI/TGUI.hpp>

void makeWindow(tgui::Gui& gui, tgui::Theme::Ptr& theme, tgui::ChildWindow::TitleButtons buttons, sf::Vector2f position, const sf::String& text)
{
    tgui::ChildWindow::Ptr child = theme->load("ChildWindow");
    child->setTitleAlignment(tgui::ChildWindow::TitleAlignment::Center);
    child->setTitleButtons(buttons);
    child->setSize(250, 120);
    child->setPosition(position);
    child->setTitle(text);
    child->connectEx("MousePressed", [&](const tgui::Callback& c){std::cout << "Clicked titlebar at: (" << tgui::to_string(c.mouse.x) << ", " << tgui::to_string(c.mouse.y)<< ")\n";});
    child->connect("Closed", [child](){std::cout << "Closed\n"; child->hideWithEffect(tgui::ShowAnimationType::Fade, sf::milliseconds(1000));});
    child->connect("Minimized", [&](){std::cout << "Minimized\n";});
    child->connect("Maximized", [&](){std::cout << "Maximized\n";});
    gui.add(child);
}


int main()
{
    sf::RenderWindow window(sf::VideoMode(1200, 676), "TGUI window");
    window.setFramerateLimit(60);

    tgui::Gui gui(window);
    gui.setFont("DejaVuSans.ttf");

    tgui::Theme::Ptr theme = std::make_shared<tgui::Theme>("widgets/Hedon.txt");

    const sf::String Long = "This is long text that is cut off at some point";

    makeWindow(gui, theme, tgui::ChildWindow::TitleButtons::None,  {64, 50}, Long);//"No Buttons");
    makeWindow(gui, theme, tgui::ChildWindow::TitleButtons::Close, {324, 50}, Long);//"Close Button");
    makeWindow(gui, theme, tgui::ChildWindow::TitleButtons::Minimize,  {584, 50}, Long);//"Minimize Button");
    makeWindow(gui, theme, static_cast<tgui::ChildWindow::TitleButtons> (tgui::ChildWindow::TitleButtons::Close | tgui::ChildWindow::TitleButtons::Minimize), {844, 50}, Long);//"Close|Minimize");

    makeWindow(gui, theme, tgui::ChildWindow::TitleButtons::Maximize,  {64, 350}, Long);//"Maximize");
    makeWindow(gui, theme, static_cast<tgui::ChildWindow::TitleButtons> (tgui::ChildWindow::TitleButtons::Close | tgui::ChildWindow::TitleButtons::Maximize), {324, 350}, Long);//"Close|Maximize");
    makeWindow(gui, theme, static_cast<tgui::ChildWindow::TitleButtons> (tgui::ChildWindow::TitleButtons::Minimize | tgui::ChildWindow::TitleButtons::Maximize),  {584, 350}, Long);//"Minimize|Maximize");
    makeWindow(gui, theme, static_cast<tgui::ChildWindow::TitleButtons> (tgui::ChildWindow::TitleButtons::Close | tgui::ChildWindow::TitleButtons::Minimize | tgui::ChildWindow::TitleButtons::Maximize), {844, 350}, Long);//"Close|Minimize|Maximize");

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            // When the window is closed, the application ends
            if (event.type == sf::Event::Closed)
                window.close();

            // When the window is resized, the view is changed
            else if (event.type == sf::Event::Resized)
            {
                window.setView(sf::View(sf::FloatRect(0, 0, event.size.width, event.size.height)));
                gui.setView(window.getView());
            }

            gui.handleEvent(event);
        }

        window.clear();
        gui.draw();
        window.display();
    }
}

It is fairly simple; the following are found in the ChildWindow class:

enum TitleButtons
{
    None     = 0,
    Close    = 1 << 0,
    Minimize = 1 << 1,
    Maximize = 1 << 2
};

void setTitleButtons(TitleButtons buttons);
sendSignal("minimized", std::static_pointer_cast<ChildWindow>(shared_from_this()));
else
{
// TODO: Texus, should an exception be thrown or just ignore this case?

This comment has been minimized.

@Ruckamongus

Ruckamongus Jun 28, 2016
Author Contributor

Just wanted to bring your attention to this and the same scenario a few lines down.

This comment has been minimized.

@texus

texus Jun 29, 2016
Owner

You can just ignore the case because it just means that the user hasn't called child->connect("minimized", ...) yet. The buttons just don't do anything in this case.

I have noticed that the else statement can be removed entirely though (and so can the 'return' statement from several lines above for the close button) because there already seems to be a return statement directly after checking these buttons.

@texus
Copy link
Owner

@texus texus commented Jun 29, 2016

Looking at the changes I see a few minor things that might have to be changed though:

  • The documentation of the ChildWindow class (comments above the ChildWindow class in the header) should include the Minimize and Maximize signals
  • The documentation of TitleButtons should not say that they have to be combined with an XOR. I would first of all use an OR (it is simpler and is the same thing as there is no overlapping) and it might be unclear how to do it without an example. The doc could just be @brief Title buttons while for every option there should be a comment next to it e.g. ///< No buttons (see Grid::Alignment for an example if it is not clear). That way the individual options will show up in the documentation. To the setTitleButtons I would then add a small example on how to use it with multiple buttons by adding the following to the docs:
/// @code
/// childWindow->setTitleButtons(tgui::ChildWindow::TitleButtons::Minimize | tgui::ChildWindow::TitleButtons::Close);
/// @endcode
  • I just noticed that the if (m_childWindow->getTheme() == nullptr) is performed twice in ChildWindowRenderer::setProperty. The second one (which throws the exception) can be removed for all these buttons (including the close button).
  • setTitleBarHeight only handles close and minimize buttons, I assume maximize is missing there?
  • Can the m_paddingBetweenButtons be put inside the renderer and loaded like m_distanceToSide instead of being hardcoded? Maybe the distance between the title text and title buttons should be m_paddingBetweenButtons instead of m_distanceToSide? That will require several lines to be changed though. (edit: or maybe it should remain m_distanceToSide or everything could even by m_distanceToSide?)
  • The if-else blocks in setTitleButtons should probably take 4 lines of code instead of 2 to conform with my code style. I suggest having the "else" and "if" on the same line. The comment could be on the line above it with optionally an empty line between the previous if body and this comment which is right before the "else if" body.
  • leftMousePressed has some tricky indentation (I haven't compiled the code yet but I'm sure gcc 6 is going to complain). The else cases of m_closeButton->mouseOnWidget(x, y) and m_minimizeButton->mouseOnWidget(x, y) should probably start a new scope.
  • Could you perhaps add a codecov.yml file in the root directory with contents comment: false? That way codecov won't start spamming on every commit in this and future pull requests. It doesn't has to be in a separate commit.
@Ruckamongus Ruckamongus force-pushed the Ruckamongus:master branch from bf2649b to a81cf93 Jun 29, 2016
@Ruckamongus
Copy link
Contributor Author

@Ruckamongus Ruckamongus commented Jun 29, 2016

I hit most of what you wanted I think. I'm not sure if I did the codecov.yml thing properly or not, sorry if not. I had some issues with git; I rarely use it and I learned "git add ." is a poor choice when you have a bat file with your password in it.

I put the m_paddingBetweenButtons in the renderer instead of getting rid of it completely. I tried to see what it would look like and the buttons seemed too distant; I like having control of them separately. I can keep this for my private use only if you'd rather though.

I just noticed that the if (m_childWindow->getTheme() == nullptr) is performed twice in ChildWindowRenderer::setProperty. The second one (which throws the exception) can be removed for all these buttons (including the close button).

I got rid of the second check and put the exception in with the first check. I wasn't sure if you wanted to get rid of the exception completely or not so I left it in for you to decide.

leftMousePressed has some tricky indentation (I haven't compiled the code yet but I'm sure gcc 6 is going to complain). The else cases of m_closeButton->mouseOnWidget(x, y) and m_minimizeButton->mouseOnWidget(x, y) should probably start a new scope.

I'm not sure I follow. I reformatted a bit to make my intention more clear. Does it still need changed?

@texus
Copy link
Owner

@texus texus commented Jun 29, 2016

It is always a good idea to run "git status" before committing to double check what you are doing.

m_paddingBetweenButtons can stay but the question that remains if the the distance between the title text and the buttons should be m_distanceToSide or m_paddingBetweenButtons. Although it probably won't matter much because the title is usually going to fit inside the title bar and these values are probably less than a few pixels in difference.

I meant that the exception should be removed as well. The if block with the exception was the original code but later the other if check was added above it to do handle the case without throwing an exception. The second if check including the exception were thus code that could never be executed and doesn't has to remain there.

The code in leftMousePressed now looks exactly like I wanted it.

I'll have a better look at the code (and test it) within the next few days.

@Ruckamongus
Copy link
Contributor Author

@Ruckamongus Ruckamongus commented Jun 30, 2016

Good to know about the "git status", thank you. I removed the exceptions hopefully that's what you meant.

As for the padding thing between the text and the buttons that's something I never considered. Like you said it's pretty trivial; I don't imagine people would be too picky about that extra pixel or two.

@texus
Copy link
Owner

@texus texus commented Jun 30, 2016

I'll probably fully test this on Saturday.

I did find one bug while building the code though (gcc gave a warning about an unused variable): you are assigning the wrong variable to m_maximizeButtonText in setTitleButtonsText.

One thing that might be discussed is the default characters for the button. Maybe 'x', '+' and '-' would look better than 'X', '^' and '_'? I didn't run the code yet so I haven't compared them yet but maybe you could have a look as well to see which characters might look better?

@Ruckamongus
Copy link
Contributor Author

@Ruckamongus Ruckamongus commented Jun 30, 2016

Ah nice catch. Copy and paste errors are the worst. I'll have to go through it all again to ensure I didn't miss anymore. Also, you're probably right about the default text. I'll change them.

@texus
Copy link
Owner

@texus texus commented Jul 1, 2016

I found one bug during testing: setTitleButtonsText does not change the text of the buttons. It should also contain the following lines:

m_closeButton->setText(m_closeButtonText);
m_minimizeButton->setText(m_minimizeButtonText);
m_maximizeButton->setText(m_maximizeButtonText);

There are still a few other minor things that I want to change in the code but I'll do them myself directly after merging.

Could you squash your commits before I merge them? Basically do the following:
git reset fa88edc (Remove all commits that you made, only the commits themselves will disappear while the changes will be kept)
git add {files that you changed} (add the changes again)
git commit -m "ChildWindow can now have a combination of minimize, maximize, and close buttons (or none at all)"
git push --force

…se buttons (or none at all)
@Ruckamongus Ruckamongus force-pushed the Ruckamongus:master branch from ec8d898 to 7bdd0cf Jul 2, 2016
@Ruckamongus
Copy link
Contributor Author

@Ruckamongus Ruckamongus commented Jul 2, 2016

I fixed the aforementioned things and squashed the commits. Thanks for the little walk through, I appreciate that! I feel like I'm creating more work for you than I'm saving.

TitleButtons m_titleButtons = TitleButtons::Close;
sf::String m_closeButtonText = "x";
sf::String m_minimizeButtonText = "-";
sf::String m_maximizeButtonText = "+";

This comment has been minimized.

@Ruckamongus

Ruckamongus Jul 2, 2016
Author Contributor

Do the button texts even need to be member variables of the class? The buttons have getters and setters so perhaps this is redundant?

This comment has been minimized.

@texus

texus Jul 2, 2016
Owner

They are also used inside the reload() function. It could be removed by changing a few other lines but I would just keep them. I'll have to rewrite the whole thing for 0.8 anyway so I'll see about what happens with these members then.

/// By default ChildWindows only display a close button. You may set the window to show a combination of buttons.
/// For example, the following will set the ChildWindow to have both a minimize and close button.
/// @code
/// childWindow->setTitleButtons(tgui::ChildWindow::TitleButtons::Minimize | tgui::ChildWindow::TitleButtons::Close);

This comment has been minimized.

@Ruckamongus

Ruckamongus Jul 2, 2016
Author Contributor

Perhaps the "tgui::ChildWindow" bit is too verbose? I think it's implicit and can be removed without changing clarity of documentation.

This comment has been minimized.

@texus

texus Jul 2, 2016
Owner

It might indeed be too verbose but this way the code can be immediately copy-pasted.
It doesn't matter for me whether it is with or without the "tgui::" part though. But if it would be decided to remove it then I would have to change the other places as well where I wrote it like that which is why I am more tempted to just leave it and keep it consistent without effort 😄.

This comment has been minimized.

@texus

texus Jul 2, 2016
Owner

That's probably the worst excuse I have ever made: I did a quick lookup and found only 8 lines that use "tgui::" as part of a parameter in the documentation 😄. The other lines are places where I use e.g. "tgui::Picture::Ptr" as type instead of "Picture::Ptr". And the one in ChildWindow is definately the longest line.

@texus texus merged commit 7bdd0cf into texus:master Jul 2, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@texus
Copy link
Owner

@texus texus commented Jul 2, 2016

Thanks again for your contribution.

I feel like I'm creating more work for you than I'm saving.

It might still be a lot of work for me but at least I have someone else thinking about the changes as well which is always good. I typically spend a lot of time on changes without even knowing whether it is going to be useful or whether people would prefer the old system so this way is better to me than having to write it myself.
One place where seeing the code that you wrote really helped was the for loop in setPosition. I would have implemented everything with just copy-pasting like you did on most places, but after having simplified that for loop I realized that I could save a lot of lines on other places. So the code eventually ended up being better than when I would have written it alone.

@Ruckamongus
Copy link
Contributor Author

@Ruckamongus Ruckamongus commented Jul 3, 2016

I know all about taking the lazy route! It's better to think of it as the route of least resistance, haha.

I'm glad the loop was helpful, and no need to thank me for contributing! You've made some awesome free software; the least the community can do is give back a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.