Add a loop count setting #218

0bmay · 2020-11-25T01:57:50Z

This changes Gifski's gif animation looping modes from none - forever ti add in an in between for looping the animation 1 - 9999 files along with the existing No loops and Forever loops.

I sent in a feature request this morning, and when I looked into what it would take, I challenged myself to add it into the awesome tool.

This code base is based off the latest master for sindresorhus/Gifski and the latest revision to kornelski/rust-ffmpeg

Prior to this change, gifski had two looping modes: off and forever. This change adds a new settings value, called loops, that will loop the animation the number of times requested.

Prior to this change, Gifski had 2 modes of looping, none or forever. This change adds in the ability to loop 1 to 9999 times

Gifski/EditVideoViewController.swift

gifski-api/gifski.h

Gifski.xcodeproj/project.pbxproj

Gifski/EditVideoViewController.swift

Gifski/Gifski.swift

sindresorhus · 2020-11-25T03:18:45Z

The textfield is too wide.
You need to add a stepper control on the right side of the textfield.
https://stackoverflow.com/questions/702829/integrate-nsstepper-with-nstextfield
The textfield should not allow negative values.
The textfield should only allow numbers.
The textfield should not be allowed to be empty.
When "loop forever" is enabled, the textfield should be disabled.
"loop forever" can be renamed to "forever".
In the Rust implementation, make sure the loop count is also passed to gifsicle, which is used when the quality slider is not 100%.

sindresorhus · 2020-11-25T03:19:35Z

I sent in a feature request this morning, and when I looked into what it would take, I challenged myself to add it into the awesome tool.

I already decline this, but seeing as you're willing to put an effort into this, I'm ok with adding it 👍🏻

reverts DEVELOPMENT_TEAM changes

sindresorhus · 2020-11-25T06:36:15Z

These labels should be vertically trailing aligned:

sindresorhus · 2020-11-25T16:18:45Z

Having any number in the loop count text field should not disable the "forever" checkbox.

Prior to this change, the timesShown variable was a little confusing This change updates the timesshown to loopCounter, adds looping support for gifsicle, and makes loopCount an optional field.

Prior to this change, gifsicle would loop forever if a loop counter of 0 was selected. This change will allow a no-looping option.

Prior to this change, UI for looping was not controlled very well This change only allows positive numbers for the loop Counter field, checking the "Forever" box disables the loop counter text field and resets the value to 0, unchecking the forever box enables the loop counter text field. UI is also updated for the looping controls.

0bmay · 2020-11-25T18:04:52Z

Sindre, thank you for your feedback. I am working on the changes.

Prior to this change, the only way to change the number of loops was to type in a value. This change adds in a stepper control to add a different way to change the number of loops of an animation. The stepper control is disabled and enabled at the same time as the loop counter textfield.

Prior to this change, variable renaming left variables with bad names This change fixes the variables to reflect the correct name

sindresorhus · 2020-11-26T03:00:59Z

The textfield should not allow negative values.

This is not fixed when using arrow keys in the text field to change the value.

sindresorhus · 2020-11-26T03:01:28Z

Is it really useful to allow numbers up to 9999? I think we should maybe set the limit at 100.

sindresorhus · 2020-11-26T03:02:13Z

There needs to be slight more margin between the text field and the stepper.

sindresorhus · 2020-11-26T03:04:11Z

If you choose a loop count, I think the preview player should loop (just like when "Forever" is checked). Ideally, it should loop according to the loop count, but not sure how easy that would be to achieve.

sindresorhus · 2020-11-26T03:08:57Z

These labels should be vertically trailing aligned:

Actually, I was wrong. It should be aligned to the slider:

Let's also rename it back to "Loop Forever" as otherwise there's just too much empty space and it looks weird.

0bmay · 2020-11-26T04:05:12Z

Is it really useful to allow numbers up to 9999? I think we should maybe set the limit at 100.

Good point, anything more than 100 (or 30+) is not much different than looping forever anyway.

0bmay · 2020-11-26T04:13:53Z

Again, thank you for the feedback.

Prior to this change, the looping using the once setting was confusing. This change modified the logic flow for looping using the once and loop_count settings. Also applies recommended changes from code review.

0bmay · 2020-11-26T04:32:12Z

The underlying gifski library works as expected for no looping, forever loops or X loops as tested via the command line tool. I need to think more on how the looping is set via the UI as right now it's not working as expected.

Prior to this change, the upper limit of looping was 9999 and the loop counter text box allowed negative numbers if arrow keys were used to change the value. This change changes the looping range to 0-100 and clamps the values to that range. This change also has UI updates as suggested by code review.

fixes the looping logic on the generated file after tweaks to the gifski library

Gifski/EditVideoViewController.swift

Prior to this change, the location of the stepper was not aligned to the top of the loop count control. The variable name for the repeatCountRange did not match the rest of the code. This change fixes the UI location of the stepper and renames a variable. Also removes a Rust warning about settings as mutable when it doesn't need to be.

sindresorhus · 2020-11-28T02:39:16Z

I believe I had it aligned center + bottom and now have it center + top.

I believe the first baseline of the stepper should be constrained to the first baseline of the text field.

I also noticed that the stepper is currently constrained to the slider, which is incorrect.

Prior to this change, UI constraints to the stepper and forever loop controls were not optimized. This change optimizes the UI constrains for the stepper and loop forever checkbox.

sindresorhus · 2020-12-16T04:31:07Z

Everything here looks good. Now just waiting for ImageOptim/gifski#151.

merge in code from gifski cli

upstream changes to gifski's library required changes to how animation looping was coded.

0bmay · 2020-12-18T01:08:14Z

updated code to use the upstream changes to ImageOptim/gifski#151.

sindresorhus · 2020-12-18T06:46:39Z

Just noticed that #218 (comment) is not done.

sindresorhus · 2020-12-18T06:53:56Z

Found a bug. When opening a video and the editor shows, the "loop count" text field is not disabled even though the "loop forever" checkbox is checked.

(In hindsight, this feature would have been much more easily implemented in SwiftUI as we wouldn't have weird state bugs like this)

fixes the state dependency on the Loop Forever checkbox and the Loop Counter textfield

0bmay · 2020-12-18T17:36:48Z

Just noticed that #218 (comment) is not done.

I think I have found a bug in NSImageView. If loading an animated gif into an NSImageView the animation will be shown at total times equal to the loop count (x) setting and not looped x times.

e.g. Loop Count is 1, the animation should be shown twice, or repeat the animation 1 time.
NSImageView sees the Loop Count as the number of times to show the animation and a value of 1 means no looping.

Also, a Loop Count of 0 in the gif is show the animation once and stop.
NSImageView sees a Loop Count of 0 as Loop Forever.

What I can do to fix the non looping issue is to not animate the image if we are not looping, but I think that would defeat the purpose of the preview window.

So how the animation is shown in an NSImaveView is not how any other tool would show the animated gif unless the animated gif's setting is loop forever.

sindresorhus · 2020-12-20T08:08:43Z

e.g. Loop Count is 1, the animation should be shown twice, or repeat the animation 1 time.
NSImageView sees the Loop Count as the number of times to show the animation and a value of 1 means no looping.

Can you report this to Apple?

So how the animation is shown in an NSImaveView is not how any other tool would show the animated gif unless the animated gif's setting is loop forever.

Yeah, I think this applies to Quick Look too. At least I know it ignores the loop count completely.

sindresorhus · 2020-12-20T08:09:37Z

Maybe we should have a warning (NSAlert) the first time the user sets the loop count in the UI to just inform them that it usually works, but the preview and Quick Look will not show the correct result?

0bmay · 2020-12-20T19:21:14Z

Issue reported to Apple Feedback ID FB8947153

I can add a one time alert when the looping/repeat value is not forever.

I know there is code out there that splits the animated gif out into individual frames and controls the loop that way, but I think that might be overkill for a screen that is shown for a short period of time and is a smaller view of the resulting animation.

sindresorhus · 2020-12-21T12:23:07Z

I can add a one time alert when the looping/repeat value is not forever.

👍 You can use:

Gifski/Gifski/EditVideoViewController.swift

Lines 274 to 282 in 5c2d437

    
           SSApp.runOnce(identifier: "fpsWarning") { 
        
           	DispatchQueue.main.async { [self] in 
        
           		NSAlert.showModal( 
        
           			for: view.window, 
        
           			message: "Animated GIF Limitation", 
        
           			informativeText: "Exporting GIFs with a frame rate higher than 50 is not supported as browsers will throttle and play them at 10 FPS." 
        
           		) 
        
           	} 
        
           }

I know there is code out there that splits the animated gif out into individual frames and controls the loop that way, but I think that might be overkill for a screen that is shown for a short period of time and is a smaller view of the resulting animation.

Agreed, that's too much.

Adds an animation preview warning that in the Conversion Completed window, the animation may not reflect the options chosen, but the exported file will be what the user expects. This is a limitation of the NSImageView control.

sindresorhus · 2020-12-22T06:00:00Z

Gifski/EditVideoViewController.swift

+				NSAlert.showModal(
+					for: view.window,
+					message: "Animated GIF Preview Limitation",
+					informativeText: "The Conversion Completed Preview after processing will not reflect the image repetitions selected. However, the exported file will refelct the options selected and repeat correctly."


I think this could be more succinctly written.

And as I commented earlier, Quick Look ignores the loop count and just always loops forever, which should also be mentioned.

"image repetitions" => "loop count"

Also typo.

sindresorhus · 2020-12-22T06:00:31Z

Gifski/EditVideoViewController.swift

@@ -299,6 +299,19 @@ final class EditVideoViewController: NSViewController {
 		}
 	}

+	private func showConversionCompletedAnimationWarningIfNeeded() {
+		// TODO: This function can be removed once NSImageView respects the looping counter in the GIF header.


Can you mirror the FB you submitted to Apple over at https://github.com/feedback-assistant/reports (follow the template) and then link to it here?

Done and thank you! I didn't know this resource existed.

This comment is now outdated. The code should not be removed when it's fixed as it also applies to Quick Look. Rather the text should be modified to only mention Quick Look when it's fixed.

Gifski/EditVideoViewController.swift

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>

added open feedback assistant link

Prior to this change, This change

sindresorhus · 2020-12-24T08:31:59Z

Gifski/EditVideoViewController.swift

+				NSAlert.showModal(
+					for: view.window,
+					message: "Animated GIF Preview Limitation",
+					informativeText: "The after conversion preview and Quick Look will may not loop as expect, but the exorted file is valid."


There are 3 typos in this text. I think it also could be more explicit about in what case it may not loop as expected. Try to spend some more effort on the text.

Prior to this change, This change

sindresorhus · 2020-12-27T09:00:37Z

This looks good now. Thank you for your hard work :)

0bmay and others added 3 commits November 24, 2020 15:52

feat: gifski cli can limit animation loops

1841740

Prior to this change, gifski had two looping modes: off and forever. This change adds a new settings value, called loops, that will loop the animation the number of times requested.

feat: max loops for conversions

dee11e7

Prior to this change, Gifski had 2 modes of looping, none or forever. This change adds in the ability to loop 1 to 9999 times

Update EditVideoViewController.swift

8eb2c2d

sindresorhus requested changes Nov 25, 2020

View reviewed changes

0bmay added 2 commits November 24, 2020 20:46

fix: revert Development Team updates

49215fe

reverts DEVELOPMENT_TEAM changes

Merge remote-tracking branch 'origin/master'

439473b

sindresorhus requested a review from kornelski November 25, 2020 16:19

0bmay added 3 commits November 25, 2020 08:48

fix: timesShown is now loopCount

2faf66f

Prior to this change, the timesShown variable was a little confusing This change updates the timesshown to loopCounter, adds looping support for gifsicle, and makes loopCount an optional field.

fix: loop count for gifsicle

50d4861

Prior to this change, gifsicle would loop forever if a loop counter of 0 was selected. This change will allow a no-looping option.

0bmay added 2 commits November 25, 2020 10:24

fix: variable fixes while updating upstream gifski

38e0981

Prior to this change, variable renaming left variables with bad names This change fixes the variables to reflect the correct name

0bmay requested a review from sindresorhus November 25, 2020 18:45

fix: logic flow change for looping in gifski library

0411ade

Prior to this change, the looping using the once setting was confusing. This change modified the logic flow for looping using the once and loop_count settings. Also applies recommended changes from code review.

0bmay added 2 commits November 26, 2020 09:38

fix: Looping Logic

13b5696

fixes the looping logic on the generated file after tweaks to the gifski library

sindresorhus reviewed Nov 27, 2020

View reviewed changes

Gifski/EditVideoViewController.swift Outdated Show resolved Hide resolved

0bmay added 2 commits November 27, 2020 18:34

Merge remote-tracking branch 'origin/master'

87612c2

fix: UI Contraints

7837ab3

Prior to this change, UI constraints to the stepper and forever loop controls were not optimized. This change optimizes the UI constrains for the stepper and loop forever checkbox.

0bmay added 2 commits December 17, 2020 15:50

feat: animation repeat

61caefa

merge in code from gifski cli

feat: animation looping updates from gifski cli

b311da4

upstream changes to gifski's library required changes to how animation looping was coded.

sindresorhus added 2 commits December 18, 2020 13:37

Update EditVideoViewController.swift

d4a9694

Update EditVideoViewController.swift

d647ba3

fix: Loop Forever and Loop Count states

b40f2be

fixes the state dependency on the Loop Forever checkbox and the Loop Counter textfield

fix: Animation Preview Warning

9bce926

Adds an animation preview warning that in the Conversion Completed window, the animation may not reflect the options chosen, but the exported file will be what the user expects. This is a limitation of the NSImageView control.

sindresorhus reviewed Dec 22, 2020

View reviewed changes

Gifski/EditVideoViewController.swift Outdated Show resolved Hide resolved

0bmay and others added 3 commits December 23, 2020 08:16

Update Gifski/EditVideoViewController.swift

cded813

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>

fix: changed gif loop warning message

0b6189c

added open feedback assistant link

fix: shortened looping warning message

359097b

Prior to this change, This change

sindresorhus reviewed Dec 24, 2020

View reviewed changes

0bmay and others added 2 commits December 25, 2020 11:26

fix: updated code comment and warning text for specific loop counts

51f6863

Prior to this change, This change

Update EditVideoViewController.swift

6e2db7f

sindresorhus merged commit c114d1f into sindresorhus:master Dec 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a loop count setting #218

Add a loop count setting #218

0bmay commented Nov 25, 2020

sindresorhus commented Nov 25, 2020

sindresorhus commented Nov 25, 2020 •

edited

Loading

sindresorhus commented Nov 25, 2020

sindresorhus commented Nov 25, 2020

0bmay commented Nov 25, 2020

sindresorhus commented Nov 26, 2020 •

edited

Loading

sindresorhus commented Nov 26, 2020

sindresorhus commented Nov 26, 2020

sindresorhus commented Nov 26, 2020 •

edited

Loading

sindresorhus commented Nov 26, 2020

0bmay commented Nov 26, 2020

0bmay commented Nov 26, 2020

0bmay commented Nov 26, 2020

sindresorhus commented Nov 28, 2020

sindresorhus commented Dec 16, 2020

0bmay commented Dec 18, 2020

sindresorhus commented Dec 18, 2020

sindresorhus commented Dec 18, 2020

0bmay commented Dec 18, 2020

sindresorhus commented Dec 20, 2020

sindresorhus commented Dec 20, 2020 •

edited

Loading

0bmay commented Dec 20, 2020

sindresorhus commented Dec 21, 2020

sindresorhus Dec 22, 2020

sindresorhus Dec 22, 2020

0bmay Dec 23, 2020

sindresorhus Dec 24, 2020

sindresorhus Dec 24, 2020 •

edited

Loading

sindresorhus commented Dec 27, 2020

Add a loop count setting #218

Add a loop count setting #218

Conversation

0bmay commented Nov 25, 2020

sindresorhus commented Nov 25, 2020

sindresorhus commented Nov 25, 2020 • edited Loading

sindresorhus commented Nov 25, 2020

sindresorhus commented Nov 25, 2020

0bmay commented Nov 25, 2020

sindresorhus commented Nov 26, 2020 • edited Loading

sindresorhus commented Nov 26, 2020

sindresorhus commented Nov 26, 2020

sindresorhus commented Nov 26, 2020 • edited Loading

sindresorhus commented Nov 26, 2020

0bmay commented Nov 26, 2020

0bmay commented Nov 26, 2020

0bmay commented Nov 26, 2020

sindresorhus commented Nov 28, 2020

sindresorhus commented Dec 16, 2020

0bmay commented Dec 18, 2020

sindresorhus commented Dec 18, 2020

sindresorhus commented Dec 18, 2020

0bmay commented Dec 18, 2020

sindresorhus commented Dec 20, 2020

sindresorhus commented Dec 20, 2020 • edited Loading

0bmay commented Dec 20, 2020

sindresorhus commented Dec 21, 2020

sindresorhus Dec 22, 2020

Choose a reason for hiding this comment

sindresorhus Dec 22, 2020

Choose a reason for hiding this comment

0bmay Dec 23, 2020

Choose a reason for hiding this comment

sindresorhus Dec 24, 2020

Choose a reason for hiding this comment

sindresorhus Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

sindresorhus commented Dec 27, 2020

sindresorhus commented Nov 25, 2020 •

edited

Loading

sindresorhus commented Nov 26, 2020 •

edited

Loading

sindresorhus commented Nov 26, 2020 •

edited

Loading

sindresorhus commented Dec 20, 2020 •

edited

Loading

sindresorhus Dec 24, 2020 •

edited

Loading