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

Fix Multiple Issues With Timestamps #8034

Merged
merged 3 commits into from
Feb 26, 2021
Merged

Fix Multiple Issues With Timestamps #8034

merged 3 commits into from
Feb 26, 2021

Conversation

commy2
Copy link
Contributor

@commy2 commy2 commented Dec 7, 2020

The pull request that added the auto timestamping to the marker menu was incredibly rushed and broken.

When merged this pull request will:

  • properly aligns text with "Timestamp" checkbox vertically
  • makes checkbox a square instead of a fucking rectangle
  • fixes completely broken 12-hour clock
  • properly handles 12:xx am (after noon) and 12:xx pm (after midnight); 00:00 am/pm is not a thing...
  • avoids the broken space character, because it is not a space in most fonts, bad workaround
  • instead, adds ace_marker_fnc_removeTimestamp function
  • removes bad code formatting changes with mid line whitespace
  • includes the vanilla header file in the common component header file instead of in 5 different functions...

before
https://i.imgur.com/B7X0tOB.png
https://i.imgur.com/mQCaqM8.png

after:
https://i.imgur.com/vkOuOiZ.png

@commy2 commy2 added kind/enhancement Release Notes: **IMPROVED:** kind/bug-fix Release Notes: **FIXED:** labels Dec 7, 2020
Copy link
Member

@jonpas jonpas left a comment

Choose a reason for hiding this comment

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

Quick check. Formatting/alignment changes are completely redundant.

addons/markers/functions/fnc_initInsertMarker.sqf Outdated Show resolved Hide resolved
addons/markers/functions/fnc_initInsertMarker.sqf Outdated Show resolved Hide resolved
@jonpas jonpas added this to the 3.13.6 milestone Dec 7, 2020

if !(_string select [1, 1] in DIGITS) exitWith {_original};
if !(_string select [2, 1] in DIGITS) exitWith {_original};
if (_string select [3, 1] != ":") exitWith {_original};
Copy link
Contributor

Choose a reason for hiding this comment

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

Fails if using "HH" format

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we just need to drop this and the next few lines to support time codes like [12]
or just drop HH format
(I'm also not sure I understand why we have "HH:MM:SS:MM" but that's unrelated)

Copy link
Contributor

Choose a reason for hiding this comment

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

@commy2 - I commited a fix to this PR, let me know if it looks ok


params ["_original"];

// @todo, 2.02 reverse command will support STRING types
Copy link
Contributor

Choose a reason for hiding this comment

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

reverse does not like unicode
image
copies as "�ссѵЀѿ� �добовС�"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

: (

Copy link
Contributor

@dedmen dedmen Feb 28, 2021

Choose a reason for hiding this comment

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

With forceUnicode? Can you please also make a FT ticket for that example if its really broken?

Copy link
Contributor

@PabstMirror PabstMirror left a comment

Choose a reason for hiding this comment

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

timecode search

@commy2 commy2 merged commit 50578a2 into master Feb 26, 2021
@commy2 commy2 deleted the fix-broken-timestamps branch February 26, 2021 23:17
@commy2
Copy link
Contributor Author

commy2 commented Feb 26, 2021

Thanks for finishing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug-fix Release Notes: **FIXED:** kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants