johndbritton / teleport

Virtual KVM for macOS
GNU General Public License v2.0
791 stars 132 forks source link

Resolves Issue #15: Unable to "Teleport" to connected screen. #29

Closed egirsova closed 8 years ago

egirsova commented 8 years ago

Added an El Capitan wallpaper so that the fix submitted in pull request #18 supports El Capitan. Addresses issue #28.

dmitshur commented 8 years ago

The change looks good to me, aside from formatting issue I mentioned above.

It might also be helpful to squash into 1 commit, for a cleaner git history.

egirsova commented 8 years ago

@shurcooL, for some reason Github makes the formatting look unclean. Everything is actually inline in the project and is consistent (or at least that's what it looks like to me when I open it with Xcode).

dmitshur commented 8 years ago

for some reason Github makes the formatting look unclean.

The reason has to do with invisible characters used for indentation, and different settings for tab width.

The code surrounding your change uses tabs for indentation. But your section uses spaces. It looks like you have 4 spaces configured as the width, and Xcode displays tabs with width of 4, hence you can't see a difference. However, GitHub displays tabs with width of 8 by default, that's why it appears out of line.

If you view on GitHub with ?ts=4 query option, it uses tab width of 4, and then it'll look the same (or almost the same, still off by 1 space because of the .diff format):

https://github.com/abyssoft/teleport/pull/29/files?ts=4

But that's still not ideal, since the source file will have spaces and tabs interchanged. See the mismatch:

image

You should either use another editor to correct the tabs vs spaces inconsistency, or change your Xcode settings to match what this project uses. Something like this, perhaps:

image

dmitshur commented 8 years ago

I can see you've squashed into a single commit, but you'll also want to rebase against the latest master of this repo. That way, the commit will contain only what you've added, without some previous changes.

egirsova commented 8 years ago

@shurcooL, I fixed the formatting in TPHost.m like you suggested so that it looks cleaner now.

Also, I did end up squashing the git commits like you suggested. I'm still new to the more complicated git functions so I wasn't entirely sure how to rebase. I struggled with it for a while (I apologize if you got a ton of emails/notifications) and I ultimately ended up just pulling from the master of this repo and fixing the conflicts. So the git history should be cleaner now although I guess its not ideal.

Thank you for all of your advice; I really appreciate it.

egirsova commented 8 years ago

@shurcooL. Update: I did a little bit more research and was successfully able to rebase! Thank you again for your help.

dmitshur commented 8 years ago

It looks great now! No problem and I'm glad you figured it out. :)

LGTM. (FWIW, since I'm just a random developer and not closely associated with this project.)