Skip to content

fix greetings function#853

Closed
leif wants to merge 1 commit intojcubic:develfrom
leif:devel
Closed

fix greetings function#853
leif wants to merge 1 commit intojcubic:develfrom
leif:devel

Conversation

@leif
Copy link
Copy Markdown

@leif leif commented Jan 31, 2023

prior to this commit, when greetings was a function, it was being called twice (and the greeting the function creates was being displayed twice).

prior to this commit, when greetings was a function, it was being called twice
(and the greeting the function creates was being displayed twice).
@jcubic
Copy link
Copy Markdown
Owner

jcubic commented Jan 31, 2023

This is an invalid and breaking change. The reason why there echo function is that inside a greeting function, you can have responsive text that changes depending on a number of columns. With this change, you no longer can have responsive text.

@leif
Copy link
Copy Markdown
Author

leif commented Jan 31, 2023

I must be misunderstanding something then. Can you show an example of defining a function for greetings where it behaves correctly?

Without this change, whenever I have a greetings function, the greeting is always displayed twice on the initial page load, and then again when the window is resized, which is quite different from the behavior when greetings is a string.

I could understand the desire to have the greeting be dynamically updated on window resize in the case that the user has not interacted with the terminal yet, but when they've already typed some commands and the greeting is off the screen I don't want it to come back! But also, of course, I don't want it displayed twice on the initial page load.

Thanks for any help you can provide (and for writing this great library)!

@jcubic
Copy link
Copy Markdown
Owner

jcubic commented Jan 31, 2023

Can you show a demo where the greeting is displayed twice?

@jcubic
Copy link
Copy Markdown
Owner

jcubic commented Jan 31, 2023

Here you have a demo of dynamic greetings: https://codepen.io/jcubic/pen/VwBEyLV?editors=1010

@leif
Copy link
Copy Markdown
Author

leif commented Jan 31, 2023

Here you have a demo of dynamic greetings: https://codepen.io/jcubic/pen/VwBEyLV?editors=1010

Thanks... I just wonder why mine doesn't work like that :)

Can you show a demo where the greeting is displayed twice?

I'll try to extract a minimal example from my project tomorrow, and hopefully I can figure out what the difference is. I'll post back here then in case it is useful.

But, from your example I am convinced that my "fix" is not correct or necessary, so I'll close this now.

@leif leif closed this Jan 31, 2023
@leif
Copy link
Copy Markdown
Author

leif commented Feb 1, 2023

@jcubic The issue is/was that my greeting function is using the callback instead of returning. I did it this way because the documentation says this:

greetings [string|function(callback)] — default is set to JQuery Terminal Signature. You can set it to string or function (like prompt) with callback argument which must be called with your string.

You can see the behavior I am seeing by changing your example code linked above to this:

const term = $('body').terminal({}, {
    greetings(cb) {
        cb(`This Terminal has ${this.cols()} cols`);
    }
});

Is this a bug, or is there some way to use the callback which I'm not understanding?

Should the documentation be updated to mention that it is also possible to return a string instead of passing it to the callback?

@jcubic
Copy link
Copy Markdown
Owner

jcubic commented Feb 1, 2023

Yes it's a bug, I didn't use callback for a long time, this is an old API that was left as backward compatibility. If you need async behavior you should make the function async or return a Promise.

@jcubic
Copy link
Copy Markdown
Owner

jcubic commented Feb 1, 2023

Created a bug report.

@jcubic
Copy link
Copy Markdown
Owner

jcubic commented Feb 1, 2023

The issue is fixed on the devel branch.

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.

2 participants