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

Settings - Add time setting type #967

Merged
merged 3 commits into from
Aug 30, 2018
Merged

Conversation

mharis001
Copy link
Contributor

@mharis001 mharis001 commented Aug 26, 2018

When merged this pull request will:

  • title, preview:
    cba_time_setting

@@ -40,5 +40,9 @@ switch (toUpper _settingType) do {
case "COLOR": {
_value isEqualType [] && {count _value == count _defaultValue} && {_value isEqualTypeAll 0} && {{_x < 0 || _x > 1} count _value == 0}
};
case "TIME": {
_settingData params ["_min", "_max"];
_value isEqualType 0 && {_value >= _min} && {_value <= _max} && {round _value == _value}
Copy link
Contributor

Choose a reason for hiding this comment

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

round _value == _value
Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the visual doesn't show decimals, checking if rounded like the default slider

Copy link
Contributor

Choose a reason for hiding this comment

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

If SLIDER does the same, then I suppose this is correct.

["Test_Setting_5", "EDITBOX", ["-test editbox-", "-tooltip-"], "My Category", "defaultValue"] call cba_settings_fnc_init;

// TIME PICKER --- extra arguments: [_min, _max, _default]
["Test_Setting_6", "TIME", ["-test time-", "-tooltip-"], "My Category", [0, 3600, 60]] call cba_settings_fnc_init;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should mention "time in seconds" somewhere.

//#define DEBUG_ENABLED_SETTINGS
#define DEBUG_MODE_FULL
#define DISABLE_COMPILE_CACHE
#define DEBUG_ENABLED_SETTINGS
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder to disable

class Hours: RscEdit {
idc = IDC_SETTING_TIME_HOURS;
style = ST_CENTER + ST_NO_RECT;
text = "00";
Copy link
Contributor

Choose a reason for hiding this comment

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

Omg I need this for the SLIDER.

@commy2
Copy link
Contributor

commy2 commented Aug 27, 2018

If the edit boxes are used instead of the slider, the control does not correctly tick "overwrite client" when altered in the MISSION tab, and does not highlight as yellow when changed.
This is done by the SLIDER setting type by using:

    // automatically check "overwrite client" for mission makers qol
    [_controlsGroup, _source] call (_controlsGroup getVariable QFUNC(auto_check_overwrite));

in the keyUp event. The other purpose of the keyUp event is to trim non number chars, which is done beautifully with the text = "00"; config entry here instead. I wonder if the entry works with commas.

@commy2 commy2 added this to the 3.8.1 milestone Aug 27, 2018
@commy2 commy2 added the Feature label Aug 27, 2018
@mharis001
Copy link
Contributor Author

Will add the line to indicate a changed value to the KillFocus event. Like the 3DEN attribute, the value does not update with a KeyUp event. A KeyUp event could be added but I think the UX might be better without it.

Also, unfortunately, the text = "00"; config entry does not trim non-number chars. That is just the default value of the edit box, I added it to test visuals when creating (not really needed anymore, so will get rid of). Add filtering of only number chars through KeyUp event?

_x params ["_idc", "_value"];

private _ctrlEdit = _controlsGroup controlsGroupCtrl _idc;
private _editText = if (_value < 10) then {format ["0%1", _value]} else {str _value};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replace with CBA_fnc_formatNumber?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could do, but this works too. Only difference I believe would be rounding instead of truncating.

@kymckay
Copy link
Contributor

kymckay commented Aug 27, 2018

I would personally remove the slider altogether for these settings because it clutters the UI.

I can't decide if I like including upper and lower limits for these 🤔 My gut says that time settings shouldn't be bound by limits (with the exception that negative time shouldn't be allowed) because otherwise why not just use a slider?

@commy2
Copy link
Contributor

commy2 commented Aug 27, 2018

SLIDER doesn't give feedback how long something takes in HH:MM:SS format.

@kymckay
Copy link
Contributor

kymckay commented Aug 27, 2018

On further consideration I think ideally the min and max should be optional for time settings. There are many use cases where you might want the user to select a time, but it doesn't matter how long/short it is.

I'd still remove the slider in the menu either way though.

@commy2
Copy link
Contributor

commy2 commented Aug 27, 2018

TIME vs DATE?

@kymckay
Copy link
Contributor

kymckay commented Aug 27, 2018

Nah, in general I'd say if you want a time from the user you wouldn't want to limit it.

Example: Cardiac arrest timer in ACE medical rewrite, prime use case for a time setting, but there's no purpose in limiting how long people should be able to set it.

@commy2
Copy link
Contributor

commy2 commented Aug 28, 2018

That's true for all sliders I suppose, but you could always make the maximum something unreasonable like 1 or maybe even 10 hours.

@mharis001
Copy link
Contributor Author

I am indifferent about including the slider but I think it is easier to change the value using it rather than the edit boxes (the 3DEN attribute has a slider).

And I agree, the maximum limit can be set to something unreasonable, which for the settings this is intended for will probably be 24 hours?

@kymckay
Copy link
Contributor

kymckay commented Aug 28, 2018

You've swayed me on the sliders 😄

Which in turn means limits make sense. No more considerations here from me 👍

@mharis001
Copy link
Contributor Author

I think this is done. Only thing maybe would adding a KeyUp event for trimming non-number chars and updating value on key press but I don't think it is necessary.

@commy2 commy2 merged commit d3d6cf3 into CBATeam:master Aug 30, 2018
@mharis001 mharis001 deleted the time-setting branch August 31, 2018 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants