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

Embeddable iTerm2Lib.framework + Demo App #366

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

lemonmojo
Copy link
Contributor

This PR adds support for an embeddable iTerm2 framework. A demo app is provided as well.

@gnachman
Copy link
Owner

Wow, this was a lot of work. Thanks for taking this on! Head explodes 🤓💥

However, I can't review a change this large. It will need to be broken into small parts. Based on what I see, it would look something like this:

  • Break out the demo app into its own PR
  • xcodeproj changes are particularly difficult to comprehend, so keep these PRs as small as possible. Maybe one for adding a shell library, then one that adds files to the library.
  • One PR containing the addition of small categories like NSWindow+iTermLib.m
  • iTermLibController should be its own PR.
  • iTermLibSessionController should be at least one PR.
  • One PR for all the changes to existing files (mostly adding #if's by the look fo it)

Changes to binary frameworks should be reverted. If there are changes to be made to them, they should be in source form only. I'll rebuild and update the binaries.

Some of this looks like it's just moving existing code from one place to another, and other bits are evidently new code. New code should use ARC. I'm in the process of ARCifying where possible, but I haven't had the courage to change the really big files yet.

I'd like to avoid the use of #if's as much as possible, preferring runtime checks or target membership instead. For example, there could be a PTYTextView+MainApp.m file that is not included in the library.

@lemonmojo
Copy link
Contributor Author

Hey @gnachman,

Although I totally understand that this PR looks overwhelming at first, the changes are mostly additive. I fear I won't find the time to break this up into so many individual PRs. Also, all of the changes depend on each other so it won't be very useful to split this up as you wouldn't be able to compile. Let me try to explain what's going on:

  • The project changes are mostly for adding the new targets and schemes (iTerm2Lib and iTerm2LibDemo).
  • I also had to introduce a new build configuration (named Library) which sets ITERM_LIB=1 in the Preprocessor Macros section of the build settings.
  • This was necessary to leave the shared projects (iTerm2Shared and iTerm2SharedARC) unchanged and just link to them from the new framework.
  • Yes, it means that I have to use #ifdefs in shared code, but it also means that any new file you add to the shared projects will automatically be usable from the new framework project without having to manually add them to the project.
  • Yes, the new code doesn't yet use ARC but that should be a mostly trivial change.
  • Everything in the iTerm2Lib folder/project is new code so there should be no consequences for the main app.
  • Same for everything in the iTerm2LibDemo folder/project.
  • There's only one kind of change to existing code: Adding #ifdefs. Most of them are just to exclude code portions from iTerm2Lib. A couple of them are to include different code in iTerm2Lib.
  • There's one fix to existing code included in https://github.com/gnachman/iTerm2/pull/366/files#diff-3fa310997c86f593b753abd61da3332bR141

That's pretty much it. Just shoot if any questions come up during review!

jkoutavas added a commit to jkoutavas/iTerm2 that referenced this pull request Sep 26, 2019
…hman#366 (gnachman#366). A key difference between lemonmojo's approach and this approach is instead of adding a iTerm2Lib target to the iTerm2 project, I've created an iTerm2Lib project and referenced iTerm2 as a sub-project, thus not requiring any modification to the iTerm2 project.
jkoutavas added a commit to jkoutavas/iTerm2 that referenced this pull request Sep 26, 2019
…nachman#366 (gnachman#366). In this commit, we execute install_name_tool on the copy of the NMSSH.framework instead of its source, thus avoiding modification of the git committed binary. Alternatively, we could build the framework as a subproject in the iTerm2 project. But for right now, we're putting off modifying the iTerm2 project.
jkoutavas added a commit to jkoutavas/iTerm2 that referenced this pull request Sep 26, 2019
 (gnachman#366). A key difference between lemonmojo's approach and this approach is instead of adding the embedded demo app as a target to the iTerm2 project, I've created an iEmbeddedDemo workspace and included the app's project and iTerm2Lib's project, thus not requiring any modification to the iTerm2 project.
@jkoutavas
Copy link
Contributor

jkoutavas commented Sep 26, 2019

Hi. As you can see by my recent commits on jkoutavas/iTerm2, I'm begun tackling the comments @gnachman (George) has made on this PR.

I'm going to do my work in two major phases:

The first phase

Provide the level of embeddding that @lemonmojo (Felix) has in his demo app -- treating an iTerm2 window as the embedding scope. I'll literally use the Demo App that Felix did as the proof harness.

Phase 1's goal is not to touch the iTerm2 xcode project or the checked-in framework binaries in any way. It will not introduce any new targets to the iTerm2 project, or mucks with existing targets.

The second phase

This phase is way more ambitious than the first and is what I need for my own project... I intend to use an "iTerm2 view" as the text renderer for v2.0 of my 22 year old MacOS MUD client, Savitar.

Phase 2 embeds iTerm2 at the view scope, which has the hosting app be responsible for coming up with a window to put iTerm2 into. This second phase will end-up removing most of the framework dependencies that the first phase has and there will be a new Demo App that demonstrates embedding an iTerm2 view into a foreign (non-iTerm2) window.

Phase 2 will include changes to the iTerm2 project. The iTerm2Shared and iTerm2ARC targets there can no longer be the targets that iTerm2Lib project draws from, for these two targets contain window and application level dependencies. There will need to be a new target (iTerm2Core?) that contains just enough modules to build an iTerm2 view, a view that does not assume an iTerm2 window, application, Sparkle support, etc. If the iTerm2Core framework target ends-up in the iTerm2 project, we may want to consider having the iTerm2 app target use iTerm2Core.framework. Whew! That's a lot of wrangling of the project. We'll have to see all the ramifications when we get into embedding phase 2.

As a means of introduction...

Thank you so much Felix for having started this process rolling. Your PR gave me the incentive to shoot for an embeddable iTerm2 and gave me a concrete example of where to start.

Thank you George for your tireless work on iTerm2, I look forward to getting to know you and the source code a lot better. :-)

@lemonmojo
Copy link
Contributor Author

@jkoutavas Awesome! Obviously we've gone through a couple of updates since my original PR and a lot of it is outdated by now.

While I probably won't find the time for another PR, we will publish our modified iTerm2 source code in the following repo after the next update to the current iTerm2 release: https://github.com/lemonmojo/RoyalTSX_V4_Public
I plan to update our modified code to iTerm2 3.3.x in October but I can't promise anything. At the moment we're still based on a commit from October 2018 (4128838).

In any case, if you have questions just @ me here!

@jkoutavas
Copy link
Contributor

jkoutavas commented Sep 26, 2019

@lemonmojo Well met. If I'm reading it right from the README at https://github.com/lemonmojo/RoyalTSX_V4_Public, have you achieved a form of my "phase 2" -- treat iTerm2 as an embedded view, and not an embedded window?

@lemonmojo
Copy link
Contributor Author

lemonmojo commented Sep 26, 2019

@jkoutavas Yes, of course! We've had this kind of integration in Royal TSX since version 1 (released 2012 if I remember correctly ;)

Also, this PR has this kind of integration. You request a new session with [iTermLibController createSessionWithProfile:command:initialSize:] which gives you a iTermLibSessionController. You then just grab the session's view with [iTermLibSessionController view] and add it to any NSWindow you want.

Check out this code in the demo app:

- (iTermLibSessionController*)createSessionInParentView:(NSView*)parentView withName:(NSString*)name andBackgroundColor:(NSColor*)backgroundColor

@jkoutavas
Copy link
Contributor

jkoutavas commented Sep 26, 2019

You then just grab the session's view with [iTermLibSessionController view] and add it to any NSWindow you want.

Hmm @lemonmojo, I made a poor assumption that because there were window-related dependencies tied to the iTerm2Lib bindings that your PR was scoped to window-level embed. I should have studied your Demo App closer.

Maybe I don't have to do the "phase 2" I listed in my comment. I could just bring "phase 1" into alignment with @gnachman's PR comments and call it a day. What do you think?

p.s. I've done a number of edits to my opening comment since you first read it, making it more readable. Have a re-read of the phase 1 and phase 2 descriptions. Thanks.

@lemonmojo
Copy link
Contributor Author

@jkoutavas Well, our goal was always to have as few changes to standalone iTerm2 as necessary, even if it means including code that isn't even used in the resulting embeddable framework/lib.

Since we've been doing this for a long time without any involvement from @gnachman or other contributors, we always had to ensure that our changes were minimal so that updating to a new commit/release didn't mean we had to redo millions of changes. Even with only the handful of changes that we made to standalone iTerm2 updating to a new release is an error-prone, multi-day process at the moment.

"Native" embeddable support would definitely be the best option and would ensure that any update is immediately usable by us or other "embedders" but that is obviously not the case right now.

@jkoutavas
Copy link
Contributor

@lemonmojo Yeah. Very understandable.

To have a fully supported "iTerm2Core.framework" that can weather main repo updates, @gnachman would have to decide to use it in the iTerm2.app itself.

George, how interesting/worthwhile is it to you to make iTerm2.app be an embedder of "iTerm2Core.framework"? Or are we making things way too burdenbsome for you to support embedders?

@gnachman
Copy link
Owner

It's not clear to me what would be in Core and what would not. It's going to be hard to make it small because dependency management has not been a priority, as I'm sure you've noticed :). That being said, as long as the increased burden is offset by architectural improvements, I think it's worth it. This is a one-man show, so I have limited time to take on big refactors.

If we do choose to go down this route it will need to be a number of small-enough-to-grok incremental changes. There's no way I can review a change as big as the original PR, and I can't merge commits that I'm not confident are correct (since I pay the price in terms of time spent dealing with issue reports).

@jkoutavas
Copy link
Contributor

If we do choose to go down this route it will need to be a number of small-enough-to-grok incremental changes.

@gnachman Yep, that's my thinking too. Incremental steps, if I end-up tackling a phase 2.

@jkoutavas
Copy link
Contributor

jkoutavas commented Oct 2, 2019

Once PR #404 is merged, the PR I'm brewing for phase 1 embedding won't have to have -DTERM_LIB=1 added to the iTerm2.xcodeproj, it can be passed along at build time (where it belongs) instead of committed to the repository. Nicely reducing the changes to the project file.

The build command can look like this: xcodebuild -project iTerm2.xcodeproj GCC_PREPROCESSOR_DEFINITIONS='$GCC_PREPROCESSOR_DEFINITIONS ITERM_LIB="1"'

Or even a more elegant way to build for the embedding library: xcodebuild -project iTerm2.xcodeproj -xcconfig iTerm_Lib.xcconfig

@gnachman
Copy link
Owner

gnachman commented Oct 3, 2019

Or even a more elegant way to build for the embedding library: xcodebuild -project iTerm2.xcodeproj -xcconfig iTerm_Lib.xcconfig

xcconfig is probably the better choice. I wouldn't be surprised to see the GCC flags deprecated.

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

Successfully merging this pull request may close these issues.

3 participants