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

Double clicking planets #194

Merged
merged 1 commit into from Jul 16, 2015
Merged

Conversation

TheSilentOne1
Copy link
Member

Left double clicking planets will open the production screen.


// open production screen
if (!m_in_production_view_mode) {
if (!m_production_wnd->Visible())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace tabs with 4 spaces

@TheSilentOne1
Copy link
Member Author

Done, should've got them all.

ToggleProduction();
CenterOnObject(planet->SystemID());
m_production_wnd->SelectSystem(planet->SystemID());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tab

@Vezzra Vezzra added the category:refactoring The Issue/PR describes or contains an improved implementation. label Jul 10, 2015
@TheSilentOne1
Copy link
Member Author

Updated

@geoffthemedio
Copy link
Member

more tabs scattered about.

@TheSilentOne1
Copy link
Member Author

Untabified.

GG::Connect(m_button_prev->LeftClickedSignal, &SidePanel::PrevButtonClicked, this);
GG::Connect(m_button_next->LeftClickedSignal, &SidePanel::NextButtonClicked, this);
GG::Connect(m_planet_panel_container->PlanetSelectedSignal, &SidePanel::PlanetSelected, this);
GG::Connect(m_planet_panel_container->PlanetLeftDoubleClickedSignal, &SidePanel::PlanetDoubleClicked, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't appear to be doing anything in SidePanel::PlanetDoubleClicked so why not connect directly to the signal as in the following 2 lines?

@TheSilentOne1
Copy link
Member Author

Function removed.

@@ -487,6 +488,10 @@ class SidePanel::PlanetPanel : public GG::Control {
* returns the id of the clicked planet */
mutable boost::signals2::signal<void (int)> LeftClickedSignal;

/** emitted when the planet is left double clicked by the user.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between LeftDoubleClickedSignal and PlanetLeftDoubleClickedSignal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LDCS is emitted by the PlanetPanel, PLDCS is emitted by the PlanetPanelContainer. This is how it is handled with (single) left as well as right-clicking. My understanding is that the signal gets passed down from PlanetPanel to PlanetPanelContainer to SidePanel, so that every class is notified of the click-event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK. Didn't notice these two additions were in separate classes.

@geoffthemedio
Copy link
Member

Looks OK, but could you squash a few of the grooming commits that were just adjusting previous commit changes in the same pull request?

@TheSilentOne1
Copy link
Member Author

I think I get your meaning, but how do I do that?

@geoffthemedio
Copy link
Member

With various git stuff? I can do it in the TortoiseGit GUI from the show log list, by selecting commits and combining them. There have been discussions of it on the forums as well for alternative means.

@TheSilentOne1
Copy link
Member Author

That should've done it.

@TheSilentOne1
Copy link
Member Author

Please don't commit yet, there's one unused function declaration, I will fix it in a couple of hours and merge into one commit again.

@geoffthemedio
Copy link
Member

Doesn't need to be just one commit if there are parts of it that would work on their own.

left double clicking planets will now open the production screen. tested
and working; squashed into one commit

Replacing tabs with spaces

Grooming (1 tab removed, 2 functions reduced to one line)

Used: Edit>Advanced>Untabify

Removed PlanetDoubleClicked function and connect signals directly.

Removed PlanetLeftDoubleClicked function

Removed unnecessary function declaration
@TheSilentOne1
Copy link
Member Author

Well in this case - overall a small commit, after initial commit just grooming - I think one commit makes good sense. Ready to merge I think?

geoffthemedio added a commit that referenced this pull request Jul 16, 2015
@geoffthemedio geoffthemedio merged commit bdeeb4a into freeorion:master Jul 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:refactoring The Issue/PR describes or contains an improved implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants