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

Nothing shows up for me #5

Closed
aminya opened this issue Aug 25, 2020 · 53 comments · Fixed by #11
Closed

Nothing shows up for me #5

aminya opened this issue Aug 25, 2020 · 53 comments · Fixed by #11
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@aminya
Copy link
Member

aminya commented Aug 25, 2020

I tried apm install and apm rebuild. No error in the console.
image

@UziTech
Copy link
Member

UziTech commented Aug 25, 2020

hmm it works for me. I did npm i && apm rebuild

Is there an error in the console?
What shell is it trying to open?

@UziTech
Copy link
Member

UziTech commented Aug 25, 2020

I added some small fixes I was working on. Rebase and run npm i && apm rebuild and see if that works.

@aminya

This comment has been minimized.

@aminya
Copy link
Member Author

aminya commented Aug 29, 2020

Any chance we get this fixed? I don't know how to test this package without being able to use it. We can ask other people to test this as well.

@UziTech
Copy link
Member

UziTech commented Aug 29, 2020

I can't debug it because it works perfectly for me. Any chance you can find an error message or put a few debugger lines in the source to see where it fails?

@UziTech
Copy link
Member

UziTech commented Aug 29, 2020

if you clone the repo and run the tests do they pass?

@UziTech
Copy link
Member

UziTech commented Aug 29, 2020

what version of atom are you using?

@aminya
Copy link
Member Author

aminya commented Aug 29, 2020

The error in #5 (comment) was for some other package.

Let me put some debugger here and there.

@aminya
Copy link
Member Author

aminya commented Aug 29, 2020

what version of atom are you using?

I have tried it with both the latest stable and the version based on atom-ide-community master.

@aminya
Copy link
Member Author

aminya commented Aug 29, 2020

if you clone the repo and run the tests do they pass?

Yes, they pass!

@aminya
Copy link
Member Author

aminya commented Aug 29, 2020

Which function is responsible for launching the x-term? I don't see anything inside the opened pane.

@aminya
Copy link
Member Author

aminya commented Aug 31, 2020

@DeeDeeG Could you test this?

@UziTech

This comment has been minimized.

@aminya

This comment has been minimized.

@DeeDeeG

This comment has been minimized.

@DeeDeeG

This comment has been minimized.

@UziTech

This comment has been minimized.

@UziTech

This comment has been minimized.

@DeeDeeG

This comment has been minimized.

@DeeDeeG

This comment has been minimized.

@aminya

This comment has been minimized.

@DeeDeeG

This comment has been minimized.

@aminya

This comment has been minimized.

@UziTech
Copy link
Member

UziTech commented Aug 31, 2020

Ok I finally got the community build working and terminal still seems to work fine for me so I am unable to debug this issue. @aminya it looks like this issue might be something specific to your setup.

Are you using pnpm to install the dependencies instead of npm? that could be the issue. When a user installs it atom will use the bundled version of npm to install the dependencies so I don't think we should use pnpm at all.

@DeeDeeG
Copy link
Member

DeeDeeG commented Aug 31, 2020

How do you get a terminal pane open? Is there a keyboard shortcut? A menu item? I've got this installed to my .atom/packages.

Thanks.

@UziTech
Copy link
Member

UziTech commented Aug 31, 2020

I used command palette terminal:open

@DeeDeeG
Copy link
Member

DeeDeeG commented Aug 31, 2020

It's weird, I don't see anything in the command palette when I type "terminal". Hmm.

I did the following in the terminal folder that I cloned:

apm install
apm link

Then I launched Atom. That should be enough to install it, right?

@UziTech
Copy link
Member

UziTech commented Aug 31, 2020

That should work. Is there a terminal folder (or symlink) in you packages directory?

@DeeDeeG
Copy link
Member

DeeDeeG commented Aug 31, 2020

Yep.

$ ls ~/.atom/packages/terminal
dist	     node_modules  package-lock.json  prettier.config.js  rollup.config.js  spec  tslint.json
LICENSE.txt  package.json  pnpm-lock.yaml     README.md		  script	    src
$ ls -l ~/.atom/packages/terminal
lrwxrwxrwx 1 [user] [group] 19 Aug 31 16:44 /home/[user]/.atom/packages/terminal -> /home/[user]/terminal

Also nothing showing up on Windows after installing. It shows in my list of community packages in Settings --> Packages, though.

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

x-terminal and terminal running together!
image

@aminya aminya added bug Something isn't working help wanted Extra attention is needed labels Sep 9, 2020
@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

The only thing I can think is an issue with pnpm.

Can you add some debugger statements to the code and figure out where it is failing?

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

I tried with pure apm.

Here is the debugger when running the open command. The set is empty for some reason!
image

@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

the set should be empty until workspace.open is called on the next line

@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

try placing a debugger in the createTerminal function in element.ts

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

It does not even hit the debugger in createTerminal

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

> (lastEntry && lastEntry.intersectionRatio === 1.0)
false

https://github.com/atom-ide-community/terminal/blob/0694079a7c445f9e15a0154ad10618806f3077e6/src/element.ts#L67

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

This patch solves the problem!!

- if (lastEntry && lastEntry.intersectionRatio === 1.0) { 
+ if (lastEntry) { 

@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

I wonder if the issue is in the intersection observer. Maybe it is not getting called.

The intersection observer is used to create the terminal once the terminal div is visible. Maybe for some reason it never becomes visible for you.

https://github.com/atom-ide-community/terminal/blob/0694079a7c445f9e15a0154ad10618806f3077e6/src/element.ts#L64-L83

@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

That patch might cause other problems try this instead

- if (lastEntry && lastEntry.intersectionRatio === 1.0) { 
+ if (lastEntry && lastEntry.intersectionRatio > 0.0) { 

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

That patch might cause other problems try this instead

- if (lastEntry && lastEntry.intersectionRatio === 1.0) { 
+ if (lastEntry && lastEntry.intersectionRatio > 0.0) { 

This works as well

@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

just curious what lastEntry.intersectionRatio is on your machine. It should always be 0.0 or 1.0

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

just curious what lastEntry.intersectionRatio is on your machine. It should always be 0.0 or 1.0

It is near to 1:

0.9984122514724731

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

Is there anything else about this? I think 7a05451 solves the problem finally!

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

Maybe we should say bigger than 0.5 so the error around 0 does not propagate into this. I don't know how this is calculated though.

@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

Weird, the threshold is set to 1 so it should only call the callback on 0 or 1.

> 0.0 should work. The only issue that could happen is something like this issue. but really what we want from that observer is to know that the terminal is not hidden (i.e. > 0).

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

If you are sure this does not become 0.001 or something this should work. This was a very strange bug fix 😅

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

Since this is a xterm issue, it might be worthwhile opening an issue and let them know

@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

That is one reason it is so weird. x-terminal works the same exact way but that worked for you and terminal did not with === 1.0

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

I opened an issue in xterm to let them know! If they use a boolean value, this will not happen.

@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

intersection observer is from chrome not xterm.

@UziTech
Copy link
Member

UziTech commented Sep 9, 2020

Intersection Observer API

@aminya
Copy link
Member Author

aminya commented Sep 9, 2020

It would be good to use the isIntersecting property and fallback to the comparison only in case it is undefined. Similar to what xterm.js does.

xtermjs/xterm.js#3085 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants