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

Sort soldiers combobox #1025

Merged
merged 26 commits into from Mar 11, 2017
Merged

Sort soldiers combobox #1025

merged 26 commits into from Mar 11, 2017

Conversation

myk002
Copy link
Contributor

@myk002 myk002 commented May 20, 2015

Adds a combobox to the bottom of the craft soldiers page to allow for sorting by various attributes.

screen shot 2015-05-18 at 5 11 35 pm

forum discussion: http://openxcom.org/forum/index.php/topic,3644.msg45136.html#msg45136
and some here: http://openxcom.org/forum/index.php/topic,3080.msg44664.html#msg44664

Modified ComboBox logic so that the list popup can be configured to appear above the combobox button if there is not enough room on the screen below. Behavior of all existing combobox widgets is unchanged.

@MeridianOXC
Copy link
Member

I see that this PR is not very active...
Could we at least merge the "don't lose scroll position when reordering" feature? It is only 2 lines of code... and such a big QoL improvement!

@MeridianOXC
Copy link
Member

I have recently cherry-picked this into my build and I have to say this is a very nice piece of code.

I have a few comments though if someone decides to merge it to master:

  1. The "drop up" combobox currently detects if it should drop up or down automatically. I changed this to a constructor parameter. It should be the developer/designer who decides. Also, prevents unwanted side effects... for example on the "New Battle" GUI. And also, as a nice bonus, State and ComboBox don't have to be friendly classes anymore.
  2. I couldn't find the reason for "_toggled = false;" added line. I removed it from my build and everything still seems to work fine.
  3. in the CraftSoldierState, all the initList() changes are obsolete; Warboy already implemented the functionality differently. Only on one place, the function needs to be called with original scroll position as parameter (at the end of sorting).
  4. In the constructor I would change "_cbxSortBy->setSelected(-1);" to "_cbxSortBy->setSelected(0);" so that the content of the combobox is displayed from the beginning (not the end) when first opened.

Lastly, big thanks again to myk002, this is a real life saver (and time saver).

@MeridianOXC
Copy link
Member

Also:
5. I have extracted the typedef, struct and macros into separate files (SoldierSortUtil.h/cpp) to reuse them on other similar GUIs; Note: extracting them in just .h file doesn't work, you need also .cpp file for implementation for some reason, so you'll need to write two macros (one for header, one for impl)

@MeridianOXC
Copy link
Member

And:
6. when Options::anytimePsiTraining is turned on, psiSkill can actually have a negative value... so the getter for this attribute needs to be written manually (just like for psiStrength); if psiSkill > 0 return psiSkill else return 0;

@myk002
Copy link
Contributor Author

myk002 commented Jan 6, 2016

@MeridianOXC I'll merge latest master and get rid of the conflicts, but could you submit a PR to my branch with your changes so I don't duplicate your effort?

incrementing expression of type bool is deprecated
warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
 _meetPointLon += 2*3.14159265358979323846 - abs(_meetPointLon);
                                             ^
                                             note: use function 'std::abs' instead
- allow "drop up" vs "drop down" functionality to be chosen by the
designer instead of being autodetected
- remove dead code
- display list from top instead of from bottom
- protect against negative psiSkill
@myk002
Copy link
Contributor Author

myk002 commented Mar 8, 2017

I've implemented all of Meridian's (excellent) suggestions, and have tested the code thoroughly with current master. Is there anything the devs would like to see for this pull request before it is merged?

@SupSuper SupSuper merged commit 43e3e41 into OpenXcom:master Mar 11, 2017
@myk002 myk002 deleted the sort_soldiers branch March 13, 2017 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants