Fix issue 157: Largest portion completed

[greysondn]

It's a little messy, but most of the duplicate code has been refactored into a parent class, FlxToolbar.

I did not try to change the constructor or refactor the code beyond a single trio of functions that were quite recurring in Adam's code. The documentation is probably not perfect, either - sorry, I only get to write in between coding breaks.

Nevertheless, this works as submitted and begins the process of fixing the code duplication and mess. Someone will have to pick up where I've left off in regards to this if there is more that is wanted to be done (fixed variables people wanted extracted for more robustness; I'd personally be tickled to see it use FlxButton or a simpler construct for the buttons and checking, etc)

[IQAndreas]

Hehe, you stole my issue. I had planned on fixing this myself, but you saved me the work. ;)

I'd personally be tickled to see it use FlxButton or a simpler construct for the buttons and checking, etc.

I had the same thought (and have a very specific solution in mind). Are you already working on this feature, or do you mind if I toss in some code that can solve this?

[greysondn]

You can go ahead. I'm done with this for now; I'm tied up in Alpha testing and Beta testing until April from the looks of it, sir :3

Perhaps best would be for maintainers to roll this in to work from or else to fork my cut; I'm still getting my feet wet with git (I grew up an SVN person ;3 ) but that would be the way I'd traditionally see it done.

I do intend to be around off and on to help with Flixel in general, however. We use it a lot.

[Dovyski]

The code looks good to me. Well done @greysondn ! :)

I've just set this pull request under the "Future release" milestone, since it's an improvement. If you guys want to work together on that now, I think @IQAndreas should fork @greysondn `s branch and send some pull request there.

Otherwise we could wait a little bit until Flixel Community dev branch gets merged into master. After that point we are free to start merging new stuff again into dev and work from that. What do you guys think?

[greysondn]

I'm still checking my email and appear to get Github notifications there (I again say I'm still working on understanding all of this). I can run basic testing and review the code before merging it into my own branch, though I may stumble here and there. I have the time needed to dedicate to that, or at least enough to be effective at doing so.

Insofar as it goes, I'd say we leave it up to @IQAndreas.

Sir - what would you like to do here?

[IQAndreas]

Sorry for the delay.

Sir - what would you like to do here?

Well, I noticed there was already a merge conflict, so I say we hold off on continuing with this part of the code until Flixel 2.56 has been released, and then throw this to the top of the list.

I'm still getting my feet wet with git [...] I can run basic testing and review the code before merging it into my own branch, though I may stumble here and there.

Great, then I'll send you the changes your way once we pick this up again and you can merge them into your branch first. It will be good practice too. :)

[greysondn]

Okay then.

If the merge conflict in particular involves this work, could you please indicate it so that I can return to my work to at least build a proper base for yours down the line?

[IQAndreas]

If the merge conflict in particular involves this work, could you please indicate it so that I can return to my work to at least build a proper base for yours down the line?

The only part that changed was the "destroy()" method: https://github.com/FlixelCommunity/flixel/pull/153#issuecomment-14321118 so fixing the conflict should be very easy, it just needs to be done manually.

There are no currently open issues that have anything to do with any of these classes, so I don't see any more changes directly being made to these before v2.56. However, we would like to follow standards and move all the classes into a src folder; I'm not sure how well GIT handles merging classes after they have been moved, so there may or may not be a merge conflict by then.

It's probably easiest to just wait and fix the conflict just before we start work on the new features (which you are welcome to do, or I am fine with doing it. :) It shouldn't be too difficult.)

[greysondn]

Proposing a plan, then, mostly as a summary of these points:

1. Wait for flixel's next milestone to be reached
2. Attempt merge of upstream changes into this fork+branch. If worse comes to worst, start a new branch and do it by hand.
3. Resolve all outstanding conflicts between the branch and upstream, which is already part of point 2, anyway.
4. Work at this feature set, whether together or merely with me working to merge in changes from your work to the branch I'll control. Which sounds more egotistic than I mean, sorry.
5. Issue a pull request and (given review and all things going well) merge all those changes back into upstream.

This already seems to be the plan - just that it's disorganized through the posts here. I'm more than happy to resolve the initial conflicts that will arise from the resolution of #22 and #153 in the upcoming milestone. As it directly affects the work I've done here, it's only the right thing to do, in my opinion.

So, if that's all there is for it, it seems like "I'll see you at the next milestone on this issue's work" is the only thing really to say here, sir. :3

[greysondn]

I'm having a bit of a tangle in my own fork repo due to the change of location, but I think I've cleaned up this patch to match the new layout. It's tracked to dev, as you can see. Pull 3 was probably pre-folder restructuring; pull 6 is definitely after - but this is only in my personal copy of dev, anyway, so it should be okay.

I would encourage a once over before merging upstream and also would like to add that the same issues are making updating my own toy branch near impossible, so I've not tested this for now.

Working on it. All of it.

[IQAndreas]

Sorry, it's probably easier if we just re-do the commits from the beginning rather than try to merge in the latest changes. It makes the commit logs clearer, and it makes the history tree look much nicer: https://github.com/FlixelCommunity/flixel/network

I went ahead and reset dev so it now branches from master at v2.56. So, checkout the current version of dev and start working from there. If you would like, I would be happy to help you out and copy over the changes based on your existing commits.

[greysondn]

As I understand, this means that the route of most utility to this project is to trash this branch and re-branch the latest dev, applying changes there.

I am okay with redoing the work.

However, this creates some fairly simple issues that I'm unfamiliar for the solutions to - pardon my inexperience here, could you teach me?

In my fork I issue localized pull requests to track upstream dev and master. When I do this, it inserts a note on the merge's occurrence. Is there a way to suppress that? (Upstream, it's borderline useless as it means "merged in changes from upstream to downstream to do this")

There is some desire to take the fork I make use of and basically redo it using any new patches I'd create (as well as any patches that would be missing from upstream that I'm interested in or do). Is there a different way to track changes in a git repo for localized use, (starting clean with a copy of upstream) than deleting the branch and/or forming a new one?

Am I understanding - regardless of the steps to get there - that this will mean that closure of this pull request and the opening of another (from the new branch) is necessary? (To put it another way, is there some magic way of adding changes to this pull request, complying with your requests, and utilizing the changed base?)

I realize it's often frustrating to rear newbies and I apologize for my inexperience in many ways; I do however have a determination to learn and presume the project itself is the best place to ask such questions when they intersect with the project's wishes for procedure. That, and I can really think of no other resource I have right now to ask such questions.

If alternative motivation is required, would the fact that tomorrow in my local time zone is my 23rd birthday and I really am overall still quite new to all of this suffice?

[IQAndreas]

... that this will mean that closure of this pull request and the opening of another (from the new branch) is necessary?

Nope, you can still use the same one! I'll walk you through it. You may know some of this already, but I'm going to include it all just for completeness sake.

First, make sure you are on the branch you are using for the pull request (in this case, it is named fix_issue_157), and then make a backup of the branch so you can check back on the changes you made. We are also going to switch to that branch temporarily.

git checkout fix_issue_157
git branch fix_issue_157_old
git checkout fix_issue_157_old

I'm assuming you haven't made any of your own changes to either master or dev, but they are instead supposed to be the same as the ones at FlixelCommunity. Next, we are going to delete those two branches, plus the current version of fix_issue_157 which we plan to replace.

git branch -d master
git branch -d dev
git branch -d fix_issue_157

I'm going to assume that origin in your repository means greysondn/flixel. We will make sure we have access to read from FlixelCommunity/flixel as well under the name FlixelCommunity, and check for any updates. We will also push those updates to origin and make sure master and dev are up to date (I'll explain what -f does later).

git remote add FlixelCommunity https://github.com/FlixelCommunity/flixel.git
git fetch FlixelCommunity
git push origin FlixelCommunity/dev:dev -f
git push origin FlixelCommunity/master:master -f

Finally, we are going to start on the new version of fix_issue_157 (that will be more up to date with dev):

git checkout -b fix_issue_157 FlixelCommunity/dev

Now you are ready to start working on all the changes you want in this existing pull request. When you are done making the changes, if you try to push origin as usual, GitHub will refuse, since it sees the two branches are too different (which is goo since you can't accidentally overwrite stuff you still needed). However, if you add -f, git will force the branch to use the new changes (which is what we want in this case):

git push origin -u fix_issue_157 -f

Your old commits on this pull request will then be replaced by any new commits you have made in your branch.

[IQAndreas]

I do apologize that I didn't do such a good job at explaining some of the commands, but hopefully the instructions are helpful anyway.

[greysondn]

(I'll explain what -f does later)

I'm a Linux user, but presumed forcing a push was a bad idea, per pretty well every person who has talked about git ever online. I didn't stop to consider how many people were actually subscribing to this particular history.

git remote add FlixelCommunity

I rewrote assuming

git remote add FlixelCommunity https://github.com/FlixelCommunity/flixel.git

After getting some error vomit and consulting the "friendly" manual. (git remote add <name> <address>)

git checkout -b FlixelCommunity/dev fix_issue_157

More error vomit and consultation of the friendly manual. git checkout -b <new_branch> <start point> ~ so the corrected form is to swap those. FC/dev is the base - not the new branch.

git push origin -u fix_issue_157 -f

I've gone ahead and done this (with effectively zero delta) as a matter of setting things up. I'll do the work as (if) I find time today.

This has been an interesting experience, to say the least of it. Thank you for your help. The corrective notes are nothing personal - a backfeed cycle and leaving the resources for anyone who would follow is a way to progress the medium ;)

[IQAndreas]

Argh, sorry about all the typos (I didn't actually test any of it, just ran off of memory), and I'm glad you found solutions for all the problems. I'll edit the fixes in in case, as you say, anyone else does the same.

I'm a Linux user

Same here, Ubuntu! What's your distro?

... but presumed forcing a push was a bad idea, per pretty well every person who has talked about git ever online.

It's like saying "Don't go over the speed limit!" 95% of the time, you should never do it; but now and again, going over the speed limit is important and can help prevent accidents.

When you push with -f, you are saying, "Forget all the commit history you know. Instead, just replace all the information with what I am giving you." If others are working from the same repo as you, that means they cannot push changes to the repository easily, since a whole bunch of old commits are either missing or replaced with other commits. It requires a lot more work on the part of the other team members in order to account for something like this.

You can see a great example of where -f becomes a problem in this pull request:

• https://github.com/FlixelCommunity/flixel/pull/178#commits-pushed-2146fda

Notice the two extra commits that I "wrote", yet they don't exist in the main repository any more. This is because while I was working on FlixelCommunity/flixel and noticed a problem in a commit I had made a few minutes earlier. Instead of adding another commit to fix it, I redid everything and forced a push with the "fixed" commits.

In that 15 minute window between when I pushed the "old" version and I forced the new fixed one, WingEraser created a branch containing those two "bad" commits. This could have been avoided if I had made sure there were no problems before pushing to GitHub.

In this case, you are forcing changes to greysondn/flixel, while everyone is working off of either their own forks or FlixelCommunity/flixel, so it's not a big deal if changes are forced to your own repo.

[greysondn]

Same here, Ubuntu! What's your distro?

greysondn@mu-tant ~/git/flixel $ lsb_release --all
No LSB modules are available.
Distributor ID: LinuxMint
Description:    Linux Mint 15 Olivia
Release:    15
Codename:   olivia

... it's not a big deal if changes are forced to your own repo.

:confused:

I didn't stop to consider how many people were actually subscribing to this particular history.

I think that sums up all I have to say about that.

I've only got one loose end left that I've not the tap-dancing faintest what to do with. greysondn/dark-flixel needs a similar reset if I value my sanity - but the current commits are of some value as they're referenced in three or four different projects (and a few proofs of concept)... That one it's okay to toy with as long as historic commits can still find the reference point for git submodule ... Any solution to that situation? (Note that there's a key distinction where the existing commits to fix_issue_157 were expendable but these aren't,)

Edit:

Actually... I'm interested in hearing that solution as it's potentially relevant in the future, but it may not be as relevant as it once was. I've reviewed the work in it and it's only patches that were placed in early upon request and a few odds and ends housekeeping-type notes. There was nothing significant that won't eventually be upstream.

[greysondn]

@Dovyski : The changeset was probably empty ..?

[IQAndreas]

The changeset was probably empty ..?

Oh, yes, sorry. I probably should have mentioned this, but GitHub acts "funny" if you push the new branch before making any changes to it (something I have accidentally done many times).

GitHub looks at your branch and says something like "Hey, look, this repository already contains 0 of 0 commits from this pull request; that's 100%! Looks like this pull request was already merged and is no longer needed!" :P

As far as I can tell from the commit history, @Dovyski never actually merged anything; it was just GitHub derping and attributing the merge to the most recent commit made in dev.

Looks like you are going to have to open a new pull request anyway. ;)

[Dovyski]

Yeah, I never merged anything regarding this issue. It's Github nonsensing :)

[greysondn]

Oh man.

While we're here, thanks for your kindness - both of you - since I first started trying to point things out and do what little I can to be of assistance. In truth, this is the first project I've joined that requires me to work with upstream work (as the other users of a repo were typically beside me or non-committing observers) and also only the second I've worked with via git. (The first being the project that led me to realizing it was to my benefit to go ahead and start presenting issues and thoughts upstream.)

Early December, then.

[Dovyski]

You're are welcome and thank you for joining us, helping with comments, pull requests and everything! :) This is also my fist open-source project involving developers other than myself. It's been pretty cool!

Keep up the great work!

[Dovyski]

@greysondn Do you still have this pull request in a branch somewhere? I want to merge it, but Github said I already did (which is a lie :)

[greysondn]

I apologize; I thought the code was still there and needed re-oriented to the codebase. I see now it's gone due to the events as we poured through this issue. We can axe it and I'll leave it off one last time and review the code once you're done with your present work to see if still needs done so that we don't have another need to rewrite as we (evidently) do now.

I am puzzled and wonder what I've forgotten, but again would like to apologize; I don't mean to waste your time.

[greysondn]

Killed my repo fork. The future should be less clinically insane.

[Dovyski]

No need to apologize :)

I've been avoiding to work on the code you and @IQAndreas discussed here, so I think I didn't (re)implemented anything you guys discussed. You can check my changes in the fix-issue-206 branch.

You are good to work on the FlxToolbar(#157) again.