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

Multi Terminals and Persistent buffers #58

Merged
merged 49 commits into from
Jun 6, 2023

Conversation

rohit-kumar-j
Copy link
Contributor

@rohit-kumar-j rohit-kumar-j commented May 28, 2023

TODO:

  • Multi terminal handling

Support

  • Linux Support
  • Windows Support
  • OSX Support

Removed

  • Child Processes

@rohit-kumar-j
Copy link
Contributor Author

rohit-kumar-j commented May 28, 2023

@Civitasv, a basic demo is ready (currently without window handling)... can you please take a look?

You need to run :CMakeGenerate twice inorder for it to work. There is no on_sucess() callback implemented yet.

@Civitasv
Copy link
Owner

I'm back to work, sorry for my late. Testing now.

@Civitasv
Copy link
Owner

In order to use this feature, I need to turn on cmake_use_terminals, am i right?

@Civitasv
Copy link
Owner

test
I modified a little for linux, now it works like this, is it expected behaviour?

@rohit-kumar-j
Copy link
Contributor Author

In order to use this feature, I need to turn on cmake_use_terminals, am i right?

Yes

As of now,there is no window handling. You can test all other settings in cmake_terminal_opts except:

    -- Window handling
    display_single_terminal_arcoss_instance = true,
    single_terminal_pet_tab = true,
    keep_terminal_in_static_location = true,

I modified a little for linux, now it works like this, is it expected behaviour?

It should resize the window to 10(height), by default... (unless you have explicitly set ti higher)

@Civitasv
Copy link
Owner

I don't know why do you use launch_task_in_a_child_process.

@rohit-kumar-j
Copy link
Contributor Author

rohit-kumar-j commented May 29, 2023

I don't know why do you use launch_task_in_a_child_process.

Haven't added multiterminal support(Each executable target has it's own terminal + 1 Main Terminal) yet.
So I've kept it for now.

@Civitasv
Copy link
Owner

Okay, I understand.

@Civitasv
Copy link
Owner

  • Fix first time :CMakeGenerate bug: ('E21: modifiable is off', does not prompt the user for build type after asking for the desired kit)

I cannot reproduce this problem.

@Civitasv
Copy link
Owner

I've added support for Linux.

@rohit-kumar-j
Copy link
Contributor Author

  • Fix first time :CMakeGenerate bug: ('E21: modifiable is off', does not prompt the user for build type after asking for the desired kit)

I cannot reproduce this problem.

It does not implement the on_success method in any way ...yet... (Not sure how to implement it)

@rohit-kumar-j
Copy link
Contributor Author

Delete the build folder and try doing a :CMakeRun directly without a :CMakeGenerate and :CMakeBuild.
The commands do not chain with any on_success() methods 😞 i.e. it first generates, then on_success(), it builds, then on_success(), it runs.(each time asking for options).

Does this happen for you?

@rohit-kumar-j
Copy link
Contributor Author

Delete the build folder and try doing a :CMakeRun directly without a :CMakeGenerate and :CMakeBuild. The commands do not chain with any on_success() methods 😞 i.e. it first generates, then on_success(), it builds, then on_success(), it runs.(each time asking for options).

Does this happen for you?

I think I will ask for a all the options from the user first(like Kit and Build Type), then do a sequentail chansend. This will be the best option, but a little tricky to implement, especially within child processes. It may be a workaround for chaining commands.

What do you think of this?

@rohit-kumar-j
Copy link
Contributor Author

rohit-kumar-j commented May 29, 2023

I've added support for Linux.

@Civitasv, I think you have missed one spot. Can you add support for this: utils.send_data_to_terminal() as well?

rohit-kumar-j and others added 2 commits May 30, 2023 05:41
utils, and now we only use terminal for execute, for other cmd like
CMakeGenerate, CMakeBuild, we use console
@Civitasv
Copy link
Owner

Civitasv commented May 30, 2023

I've extracted the terminal logic to terminal.lua, and I think it is not necessary to use terminal when generating and building, but I still keep those codes in terminal.lua(in terminal.run function). Now utils.execute will invoke terminal.execute, which will send command to our unified terminal. What do you think?

Peek 2023-05-30 12-12

@rohit-kumar-j
Copy link
Contributor Author

And, what's the difference between check_cmake_buffers_are_displayed_in_current_tab and check_if_cmake_buffers_are_displayed_in_current_tab

Sorry, wrong naming. Pushed a fix. 700c450

@rohit-kumar-j
Copy link
Contributor Author

The problem is: the first time I enter cmake project and invoke CMakeBuild, it will call CMakeGenerate correctly. And after finishing CMakeGenerate, it also correctly calls cmake.build, but this time, it cannot get build targets correctly.

What is the config for this?
Do you use cmake_always_use_terminal = true for this?

@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

Yes.

@rohit-kumar-j
Copy link
Contributor Author

rohit-kumar-j commented Jun 6, 2023

I encountered (a similar/the same?) problem...

First Try

When I do a :CMakeRun on new project(unconfigured, no build directory), it chains the following:

  1. :CMakeGenerate
  2. :CMakeClean
  3. cmake -E create_symlink ...json ...json
  4. cmake -E create_symlink ...json ...json

Config:
cmake_always_use_terminal = true,


Second Try

When I do a :CMakeRun on a configured and built project, but Vim Just Entered (:h VimEnter), it detects the project's targets, but does not create a new terminal with the incorrect name:
[CMakeTools]: null. It does not eagerly detect the names of the targets.

Config:
cmake_always_use_terminal = true,

@rohit-kumar-j
Copy link
Contributor Author

rohit-kumar-j commented Jun 6, 2023

I have an idea.

For a new project, for the first time, when we do a :CMakeGenerate, it configures the project.
After configuring, only then we get a CMakeCache.txt. So, we can have a loop that detects if a CMakeCache.txt file is created in the build directory. Only after this it is configured, we can use the codemodel targets.

If this is done in async, I think there might not be too many calls to the filesystem, so as to block the IO. If we get a hit on the CMakeCache.txt, we can raise a flag and end the loop. This triggers the get_targets...() functions.

Although this is a CMake specific thing, I tested this for both MSVC(VS Generator) and Clang(Ninja). The CMakeCache.txt seems to be generated only after the configure step is complete.

@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

I encountered (a similar/the same?) problem...

Kinda same. But for your second try, it works correctly on Linux.

@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

I have an idea.

For a new project, for the first time, when we do a :CMakeGenerate, it configures the project. After configuring, only then we get a CMakeCache.txt. So, we can have a loop that detects if a CMakeCache.txt file is created in the build directory. Only after this it is configured, we can use the codemodel targets.

If this is done in async, I think there might not be too many calls to the filesystem, so as to block the IO. If we get a hit on the CMakeCache.txt, we can raise a flag and end the loop. This triggers the get_targets...() functions.

Although this is a CMake specific thing, I tested this for both MSVC(VS Generator) and Clang(Ninja). The CMakeCache.txt seems to be generated only after the configure step is complete.

Yeah, that’s a way to fix it. But it is a really weird problem.

@rohit-kumar-j
Copy link
Contributor Author

Before doing this, I think we can do some clean up and merge PR. 😅

Can you add the description of the terminal options in the README?
I'm not sure what to write. There was so much in this PR.

@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

I think launch_task_in_a_child_process and launch_executable_in_a_child_process can be removed.

@rohit-kumar-j
Copy link
Contributor Author

I agree.

@rohit-kumar-j
Copy link
Contributor Author

Let's keep it internally?

@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

Also remember remove the logic about child procs in terminal.lua

@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

Let's keep it internally?

Don’t think it will be useful someday.

@rohit-kumar-j
Copy link
Contributor Author

Completely remove them or store them in a .legacy.lua file that isn't required by any of the other files?

@rohit-kumar-j
Copy link
Contributor Author

Let's keep it internally?

Don’t think it will be useful someday.

Okay

@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

Before doing this, I think we can do some clean up and merge PR. sweat_smile

Can you add the description of the terminal options in the README? I'm not sure what to write. There was so much in this PR.

I will add it today.

@rohit-kumar-j
Copy link
Contributor Author

rohit-kumar-j commented Jun 6, 2023

There seems to be problem with

  vim.wait(10000000, function()
    if not terminal.has_active_job() then
      -- it should have an active job
      return false
    end
    return true
  end, 10)

  vim.wait(10000000, function()
    if terminal.has_active_job() then
      -- it should not have an active job
      return false
    end
    return true
  end, 10)

I have to do CTRL C twice for seeing any output from neovim. It is blocking even the viewport.
This is for projects with and without the pre-configuration.

@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

Yes, I don't know hot to make it async yet.

@rohit-kumar-j
Copy link
Contributor Author

rohit-kumar-j commented Jun 6, 2023

Yes, I don't know hot to make it async yet.

Although not fully async according to TJ, this can be a starting point.

I'm trying to use this.

This is only possible for configure with the CMakeCache.txt and not for fully chainning commands.

@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

Okay... Maybe we should find another way. I will update README first.

@Civitasv Civitasv merged commit 6261df4 into Civitasv:master Jun 6, 2023
1 check failed
@Civitasv
Copy link
Owner

Civitasv commented Jun 6, 2023

Make a new issue, we can continue discuss there. #60

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.

None yet

2 participants