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

Improve statusbar #125

Merged
merged 5 commits into from
Mar 8, 2016
Merged

Improve statusbar #125

merged 5 commits into from
Mar 8, 2016

Conversation

charlielee
Copy link
Owner

I've made the status bar more compact and less "wordy":

Before:
beforestatusbar

After:
statusafter

I've also changed a couple of things that were bugging me:

Renamed curFrame

Firstly I've renamed the variable curFrame to totalFrames. This was getting really confusing for me because curFrame didn't really reflect the variable's purpose (which is to provide the total number of frames captured). Also the variables curSelectedFrame and curPlayFrame were similarly named to curFrame but act quite differently to it since they change value relatively frequently while curFrame is more of a constant. In addition, the variable statusBarCurFrame was, despite its name, often not equal to var curFrame

Changed frame number when in capture mode

When in capture mode the frame number in the status bar is now 0. It was previously equal to the number of frames captured (or the old curFrame) which was confusing because the last frame captured would also display a frame number of curFrame.

There is a debate to be had that instead of 0 the frame number in capture mode should one more than the frame total (ie 19 in the example above) - which is how Dragonframe works. However, I personally find this quite confusing and in this proposed form the status bar would say frame 19 of 18 which does sound rather clumsy...

Charlie Lee added 2 commits March 1, 2016 17:10
This makes the variable's contents far less confusing.
Also in capture mode the status bar displays frame 0 rather than the
total number of frames.
@le717
Copy link
Contributor

le717 commented Mar 1, 2016

Visually looks nice, will look over code and review the best I can on mobile ASAP.

@@ -165,8 +165,7 @@ <h2 id="confirm-title"><i class="fa fa-exclamation-triangle"></i> Confirm</h2>

<div id="statusBar">
<ul>
<li id="currentFrame">Current frame: <span>0</span></li>
<li id="num-of-frames"><span>0 frames</span> captured</li>
<li>Frame <span id="currentFrame">0</span> of <span id="num-of-frames">0</span></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Go ahead and change this ID to current-frame since you are already editing it.

@le717 le717 added the feature New or enhanced functionality for users label Mar 2, 2016
@le717
Copy link
Contributor

le717 commented Mar 2, 2016

Firstly I've renamed the variable curFrame to totalFrames. This was getting really confusing for me because curFrame didn't really reflect the variable's purpose (which is to provide the total number of frames captured).

+1

Also the variables curSelectedFrame and curPlayFrame were similarly named to curFrame but act quite differently to it since they change value relatively frequently while curFrame is more of a constant. In addition, the variable statusBarCurFrame was, despite its name, often not equal to var curFrame

So do these need to be renamed too?

There is a debate to be had that instead of 0 the frame number in capture mode should one more than the frame total (ie 19 in the example above) - which is how Dragonframe works. However, I personally find this quite confusing and in this proposed form the status bar would say frame 19 of 18 which does sound rather clumsy...

That does sound odd but 0 might sound worse. What does other software do in this matter? Also, don't we update the second value in capture mode? Then why not, while in capture mode, update both values to be exactly the same and always remain in sync (Frame 19 of 19, Frame 20 of 20, etc).

@charlielee
Copy link
Owner Author

So do these need to be renamed too?

No these have names that make sense :)

What does other software do in this matter?

Well:

Dragonframe in Live view

dragonframe

  • The frame num at the bottom in live view is one more than the total number of frames captured.
  • The number at the top is also one more than the num of frames captured. It remains constant during playback.

iStopMotion

In playback mode:

In live view:

A frame number is simply not shown in capture mode in iStopMotion. (I think...)

Also, don't we update the second value in capture mode?

The 2nd number is updated when a picture is taken or deleted. When in capture mode and before a picture is taken saying frame 19 of 19 doesn't make sense to me because if I've only taken 18 pictures why does the program say there's 19 in total. Perhaps this makes more sense to seasoned Dragonframe users, but I think it's weird!

@le717 I've decided after looking at the other programs that (with 18 pics captured and in capture mode) instead of saying Frame 0 of 18 the status bar should say frame 19 of 18. Sure it's clumsy but it's chronological.

Also thanks for reviewing my code here, I'll push a commit soon :)

@charlielee charlielee mentioned this pull request Mar 2, 2016
@le717 le717 added this to the v0.6.x milestone Mar 2, 2016
@le717
Copy link
Contributor

le717 commented Mar 4, 2016

Anytime you have this ready, I'm ready. :)

@charlielee
Copy link
Owner Author

@le717 I've just pushed some changes. The status bar frame number is updated using a new function called _updateStatusBarCurFrame (arguably a bit of a long name). In live mode the status bar now displays one more than the total number of frames captured (ie frame 19 of 18).

@le717
Copy link
Contributor

le717 commented Mar 8, 2016

Code looks good. We now suddenly have a merge conflict. Follow the directions GitHub provides (see image) and let me know when you've got that done.

link

@charlielee
Copy link
Owner Author

@le717 the conflicts have been resolved!

@le717
Copy link
Contributor

le717 commented Mar 8, 2016

No dude, you resolve conflicts, not bring them back. :P

le717 pushed a commit that referenced this pull request Mar 8, 2016
@le717 le717 merged commit 5e7601e into master Mar 8, 2016
@le717 le717 deleted the improve-statusbar branch March 8, 2016 20:13
…e-statusbar

# Conflicts:
#	app/animator.html
#	app/js/main.js
@charlielee
Copy link
Owner Author

@le717 ah you caught me before I edited it! ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New or enhanced functionality for users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants