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

Issue 1074. Slider to zoom framesize at timeline. Option removed from prefs. #1121

Merged
merged 2 commits into from Dec 9, 2018

Conversation

Projects
None yet
5 participants
@davidlamhauge
Copy link
Contributor

davidlamhauge commented Nov 2, 2018

A zoom slider to change frame size.
Values from 4 to 40. (Was 4 to 20).
For some reason it rearranges layout? Besides from that it works fine.

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Nov 2, 2018

Aside from the fact that this offset the playback buttons from being centred in the timeline, I'm not sure we should add features to the current timeline, since it's going to be reworked.

@scribblemaniac started a timeline rewrite quite a while ago, which radially changes the implementation of the timeline, so any new features we add here will have to be migrated.

your implementation seems to follow the direction of my mockup though so that's nice, but as it stands currently this requires the timeline to have a an even wider width to allow to see all buttons, which I'm not sure we want right now. The proper way to fix this would be to rethink the layout of the timeline and make it fit on less space but I personally think that should be done in the rewrite rather than the current timeline.

it looks good though @davidlamhauge but I wouldn't want to see it merged just to see it possibly go away in the rewrite.

for those that haven't compiled:
feature

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Nov 2, 2018

@CandyFace @scribblemaniac I'm a bit worried about the following though. We spoke about it over discord too, and even tried to give a general consensus . I simply don't think postponing useful features in lieu of the "we're going to rewrite it anyway" excuse we've been giving to ourselves for the past years is the way to go anymore. I'll be frank. I'm not seeing that timeline re-write happening in at least 2 years time from now and that is a generous estimate.

It's not because there isn't progress already, but because there's simply too much to do before getting it "right". However If we inhibit the program from growing visibly for our users, they simply won't be interested in coming back to it, and we'll lose traction. Yes, it might be a waste of time to improve the current timeline in the long run, but I'd rather have what we can implemented to be used now, so we can get more people using the software, spreading good will, and then once we have all the foundation elements ready target a full re-write of that feature along any other topping that might have been added to it. Let me know what you think.

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Nov 2, 2018

@Jose-Moreno I understand what you mean, I'm not sure it's an excuse though since (I assume) we all want it to happen. The more we built on top of the current timeline, the harder and more time consuming it'll become to make anything that can replace the current one.

I should probably mention that I don't work on the ethic that the application might lose user popularity, so we should just hack away to please users. That's what i'm doing at the job, because I'm paid to do so and is constrained by deadlines. When I work on pencil however I believe that I can take my time to make things properly, even if it means that users will have to wait even longer. In that process I hope to make less buggy and hacky code too that can be easier maintained in the future.

Working on a foundation and later do a re-write might sound ok in theory, but through my experience, in practice that doesn't work in software development because too many things might get coupled together by that time, thus making it even harder to take the stubs apart to rewrite. There are simply too many wires to untangle at that point that you'll be spending more time reading code, trying to understand how to decouple x and y again and fix unwanted bugs from that, than writing new code.

When that's said...
There's been no progress on the re-write in a very long time so maybe it is time to simply say it's been inactive for too long to consider waiting anymore and therefore let people build on top of the current.

@davidlamhauge

This comment has been minimized.

Copy link
Contributor Author

davidlamhauge commented Nov 2, 2018

I can't decide whether it's going to be implemented or not. I just saw it as an issue (#1074) raised less than 5 weeks ago, so I thought I'd implement it, since no one else had done it through Hacktoberfest.
If you don't want it, then don't merge it. It's 1-2 hour of work. No big deal.

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Nov 2, 2018

@davidlamhauge Sorry, didn't mean to spark controversy here. Neither of us certainly want to waste precious contributors work. Thank you for your understanding. Leave the PR up while we deal with this conundrum first and thanks again for your contribution.

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Nov 2, 2018

@CandyFace I agree. I do not want to make an unmanageable mess that we have to untangle later. I also agree that we shouldn't exactly work only to "please" users, but at the same time I get inclined to disagree with that statement as much as I agree with it, mainly because we are invariably making a software that other people will use; non-technical users at that. Of course this thought is a balancing act as much as everything in software development; not an easy thing to manage, but we'll get there 😋

I'm trying to abstract and analyze all of the information we have about this, but however I look at it we need to get to a definitive decision on this topic (the timeline re-write) before moving on, it's simply such an important part of the program that we kind of have to schedule it "soon". Also apologies for my poor choice of words, perhaps "excuse" wasn't what I meant to say but it's more that It feels like this is such an impassable obstacle of a dependency for the growth of the software that it needs to be addressed.

Of course I don't mean to make this that big of a deal. We simply have to clarify how much of a priority the timeline should be. If possible I'd like us to discuss this with @chchwy and the others that have been notified about it. Of course everyone should work in whatever feature or fix they want, but if those contributions become blocked by this or any other dependency, we need to remove it from the way. Let me know what do you, or anyone else that reads this, think of how we should discuss this issue in the near future to consider getting at least a milestone roadmap for it's future implementation considering we have mockups and a possible (?) prototype code-base.

As always thank you all for your input 🙂

@davidlamhauge

This comment has been minimized.

Copy link
Contributor Author

davidlamhauge commented Nov 3, 2018

I really mean it @Jose-Moreno , when I say that it is no big deal. I saw the issue when it was posted 4-5 weeks ago, and thought that I would implement it, if no one else did it during Hacktoberfest. I had a couple of hours yesterday, and I implemented it.
I can understand @CandyFace 's caution, about adding to much to a feature that is going to be rewritten, and I don't know where this will end. I trust you'll find the best solution.
This issue has three labels; 'discussion', 'enhancement' and 'timeline'. Does 'discussion' mean that it shouldn't be implemented? If so, I'll suggest a more precise wording, like 'Implementation on hold' or something.

@CandyFace
Copy link
Member

CandyFace left a comment

Looks good, my only request is change of wording. It should be frameWidth instead rather than size, since we're only adjusting the width of the containers.

Adjusted tool tip from 'frame size' to 'frame width'
Co-Authored-By: davidlamhauge <davidlamhauge@gmail.com>
@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Nov 3, 2018

@davidlamhauge Thanks for the vote of confidence 😉 As for "discussion" it means exactly this. Since each of us has different perspectives on how an issue should be solved, or how a feature should behave, often times we get to different conclusions as to how a functionality should work, therefore when a topic is contested as such we use the "discussion" label to signal that the issue has conceptual or technical inaccuracies to sort, and we do so either on the report or PR's related to it (like we are doing right now) we also try to discuss outside github as you've seen but it's good to leave the discussion history here so future contributors can understand our choices.

@scribblemaniac
Copy link
Member

scribblemaniac left a comment

Looks good except where noted. A zoom slider is in the timeline plans anyway so it's not a big deal to move it into there now.

@chchwy chchwy added this to the 0.6.3 milestone Dec 4, 2018

@chchwy chchwy merged commit d2c5eaa into pencil2d:master Dec 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

scribblemaniac added a commit to scribblemaniac/pencil that referenced this pull request Feb 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.