search

lxsang / PTerm

MIT License
35 stars 8 forks source link

Various fixes and improvements #54

Closed janvrany closed 1 year ago

janvrany commented 1 year ago

This PR contains various fixes and improvements I've done when trying to make PTerm working in my environment / my usecases.

Fell free to cherry-pick what you like and leave what you do not like (but please preserve commit authorship)

Rinzwind commented 1 year ago

Something to keep in mind here: POSIX.1-2017 does not specify POSIX_SPAWN_SETSID as one of the possible flags for posix_spawnattr_setflags. The GNU C Library version 2.26 availability announcement said it “is scheduled to be added to the next major revision of POSIX”. On macOS 12.6.8, the ‘man’ page for the function does not mention the flag, though it is defined in ‘spawn.h’.

janvrany commented 1 year ago

@Rinzwind Right. I'm personally fine with that - especially as the alternative is to require full working toolchain on deployment (not only development) machine - but sure, I'm not the one to decide. I have not checked on macOS since I do not have access to it.

lxsang commented 1 year ago

I've added 2 commits to fix some issues with MacOS.

I did a quick test, it works. But we may need to add some Unit tests for the added functions.

I notice occasionally some unstable behaviors on window resize or copy/paste (i.e. copy by selecting test and paste by right click)...

janvrany commented 1 year ago

@lxsang

@janvrany Could you please add two unit tests to the following methods on LibPTerm:

Done in 70b7608.

janvrany commented 1 year ago

However, I'm not happy with using Smalltalk os isLinux instead of my original IsLinux class variable, since #isLinux does not exist on Pharo 8.

lxsang commented 1 year ago

However, I'm not happy with using Smalltalk os isLinux instead of my original IsLinux class variable, since #isLinux does not exist on Pharo 8.

After the discussion in https://github.com/lxsang/PTerm/pull/38#issuecomment-1207255401 and the PR #39, we've decided that the master branch shall support only pharo 9 and up. We keep a separated branch for older versions https://github.com/lxsang/PTerm/tree/pharo7-stable.

Support for Pharo 7 is on the branch pharo7-stable, however this branch should only accept PR on bug fixs but no new feature will be added.

Although the branche is named pharo7-stable, it should be compatible with pharo 8.

You seam make the keyboad handling worked on Pharo 8, but i'm not sure about the compatibility of the rest

janvrany commented 1 year ago

After the discussion in https://github.com/lxsang/PTerm/pull/38#issuecomment-1207255401 and the PR https://github.com/lxsang/PTerm/pull/39, we've decided that the master branch shall support only pharo 9 and up.

Fair enough. I'm happy maintaining my own fork. For us Pharo 8 is a must.