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

X Mode (rebased) #137

Merged
merged 11 commits into from
Jun 1, 2015
Merged

X Mode (rebased) #137

merged 11 commits into from
Jun 1, 2015

Conversation

heyitsxavier
Copy link

Rebased previous x mode + nits + small test updates.

@lastquestion
Copy link
Contributor

I think there's a very small bug where if you're in X mode and you enter command mode, you'll be back in select mode when you are back on the main choose screen. Also I think the color of the text is white only because we are printing the scrollbar chrome right before, so the default color is set. It's not necessary but would be very good to use the color printer to affirmatively default to the default foreground color. (whew). I could do that in my cleanup PR #139 if @pcottle merges this first.

@pcottle
Copy link
Contributor

pcottle commented May 26, 2015

I think there's a very small bug where if you're in X mode and you enter command mode, you'll be back in select mode when you are back on the main choose screen.

yeah I could see that happening -- we probably need to track some kind of lastMode variable to keep track of where we came from

Also I think the color of the text is white only because we are printing the scrollbar chrome right before, so the default color is set. It's not necessary but would be very good to use the color printer to affirmatively default to the default foreground color. (whew).

yeah do you mind doing this @xavierbeynon and possibly adding a test for X-mode as well? It should be easy if you just provide x as one of the mocked inputs to the test case and check for the right screen. that way we can see what this looks like

@@ -370,6 +381,8 @@ def processInput(self, key):
# before exiting the program
self.getFilesToUse()
self.cursesAPI.exit()
elif self.mode == X_MODE and key in lbls:
Copy link
Contributor

Choose a reason for hiding this comment

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

this might prone to breakage in the future since we might add keys above this statement that are in lbls and forget about X_MODE. i wonder if we could instead refactor some of this...

@heyitsxavier
Copy link
Author

Sorry for being late guys - I'm between situations right now. I could probably write a test this weekend and refactor the keys thing. I don't really want to do anything with color printer since I don't know what that is.

@pcottle
Copy link
Contributor

pcottle commented Jun 1, 2015

Sorry for being late guys - I'm between situations right now. I could probably write a test this weekend and refactor the keys thing. I don't really want to do anything with color printer since I don't know what that is.

its cool! let me see if i can merge this all in

@pcottle pcottle merged commit f095482 into facebook:master Jun 1, 2015
pcottle added a commit that referenced this pull request Jun 1, 2015
pcottle added a commit that referenced this pull request Jun 1, 2015
pcottle added a commit that referenced this pull request Jun 1, 2015
pcottle added a commit that referenced this pull request Jun 1, 2015
@pcottle
Copy link
Contributor

pcottle commented Jun 1, 2015

Alright sweet! Thanks @xavierbeynon for the PR, rebasing was quite trivial (just a merge conflict in an expected test output file).

Now that I'm playing around with this locally it actually is quite fun / useful :)

we should try to figure out how to turn off the cursor in x mode -- one more thing to fix

pcottle added a commit that referenced this pull request Jun 1, 2015
@pcottle
Copy link
Contributor

pcottle commented Jun 1, 2015

Fixed! Alright I think this is all wrapped up now, thanks again 🎉 👏 this should make it into the 0.6.1 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants