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

TGCommandLinePlugin Up-Down Arrows and Tab Keys #7180

Closed
ferdymercury opened this issue Feb 11, 2021 · 10 comments
Closed

TGCommandLinePlugin Up-Down Arrows and Tab Keys #7180

ferdymercury opened this issue Feb 11, 2021 · 10 comments

Comments

@ferdymercury
Copy link
Collaborator

ferdymercury commented Feb 11, 2021

Explain what you would like to see improved

The TGCommandLinePlugin is great for external CMake/C++ applications that use some ROOT classes / TApplication and GUI classes TGFrame, where you want to have some interactive debugging options via Cling. To make it even more user-friendly, it would be nice if the following things were improved:

  • Pressing Up and Down Arrow Keys changes the selected command in the ComboBox.
  • Pressing Tab Key propagates to the command line to give command suggestions / autocompletion, as it happens in the real ROOT terminal
  • Typed commands are added to the ROOT history file, so that the next time

Optional: share how it could be improved

Changes in TGTextEntry:

    virtual  void        UpDownArrowsPressed(Int_t);                           //*SIGNAL*

case kKey_Down:
          CursorOutDown();
          UpDownArrowsPressed(1);
          break;
case kKey_Up:
          CursorOutUp();
          UpDownArrowsPressed(0);
          break;

Changes in TGCommandLinePlugin

fCommand->Connect("TabPressed()", "TGCommandPlugin", this,
                      "HandleTab()");
fCommand->Connect("UpDownArrowsPressed()", "TGCommandPlugin", this,
                      "HandleArrows()");

void TGCommandPlugin::HandleTab()
 {
  //redirect CLI suggestions back to terminal
}


void TGCommandPlugin::HandleArrows()
 {
  //Scroll one up or down in combobox
}

if (app->InheritsFrom("TRint") || fForceFlag ) // accept also if a user set an fForceFlat explicitly?
          Gl_histadd((char *)string);

To Reproduce

  • Open tutorials/gui/guiWithCINT.C
  • Try to use up and down arrow keys to scroll between line commands

Setup

  1. ROOT 6.23/01
  2. Linux
  3. Self-built
@github-actions github-actions bot added this to Needs triage in Triage Feb 11, 2021
@eguiraud eguiraud removed this from Needs triage in Triage Feb 12, 2021
@bellenot
Copy link
Member

  • Pressing Up and Down Arrow Keys changes the selected command in the ComboBox.

I'll take a look if and how that could easily be implemented

  • Pressing Tab Key propagates to the command line to give command suggestions / autocompletion, as it happens in the real ROOT terminal

Sorry, but I don't think I will embark in such development in the current GUI.

  • Typed commands are added to the ROOT history file, so that the next time

I'll cross-check, but I think it should already be the case...

bellenot added a commit to bellenot/root that referenced this issue Feb 16, 2021
@bellenot
Copy link
Member

bellenot commented Feb 16, 2021

@ferdymercury please check root-project#7180

@ferdymercury
Copy link
Collaborator Author

ferdymercury commented Feb 16, 2021

Thanks so much.

I checked it, it works great, I saw just a couple of issues:

  • If I go all the way down to the last entry, I need press twice 'up' key to go the second entry. Probably, some issue between index 0 and index 1.
  • Maybe, index 0 should correspond to the 'empty' comannd line window, or whatever the user had typed in the beginning before pressing the arrows. For example, if (te) { fCommand->SetText(te->GetText()->GetString()); } else { fCommand->SetText(temporaryString); }, where temporaryString was whatever was in the TGTextEntry before scrolling in the history. Maybe const char *string = fCommandBuf->GetString(); ?
  • The ROOT history is still not saved even if I call plugin->SetHistAdd(); which means that somehow Gl_histadd((char *)string); is not working in a standalone CMake TApplication?

Thanks for the excellent support.

@bellenot
Copy link
Member

  • If I go all the way down to the last entry, I need press twice 'up' key to go the second entry. Probably, some issue between index 0 and index 1.

I'll check

  • Maybe, index 0 should correspond to the 'empty' comannd line window, or whatever the user had typed in the beginning before pressing the arrows. For example, if (te) { fCommand->SetText(te->GetText()->GetString()); } else { fCommand->SetText(temporaryString); }, where temporaryString was whatever was in the TGTextEntry before scrolling in the history.

OK, I'll see what I can do

  • The ROOT history is still not saved even if I call plugin->SetHistAdd(); which means that somehow Gl_histadd((char *)string); is not working in a standalone CMake TApplication?

That might well be. But if it's the case then I'm afraid there is not much I can do..

@ferdymercury
Copy link
Collaborator Author

ferdymercury commented Feb 16, 2021

Thanks!

For the tab-completion, I checked and it looks it could be done in a more or less simple way I guess (pseudocode):

fCommand->Connect("TabPressed()", "TGCommandPlugin", this, "HandleTab()");

void TGCommandPlugin::HandleTab()
{
   const std::string line = fCommandBuf->GetString();
   const size_t cur = fCursorIX; //
   std::vector<string> result;
   gInterpreter->CodeComplete(line, cur, result);
   for(auto& res : result)
   {
       //todo append to output window
   }
}

Note: inspired by TClingTabCompletion::Complete function.

@bellenot
Copy link
Member

bellenot commented Feb 17, 2021

@ferdymercury I updated the PR. Please check and let me know. Thanks!

@ferdymercury
Copy link
Collaborator Author

ferdymercury commented Feb 17, 2021

Looks really great, thank you very much.

There is only one issue. The history is not being added. I tracked it down to here:

image

The command is about to be added, but the history file name is empty, thus it returns on line 73.

fContext->GetHistory() on TextInput.cpp has an empty history filename.

I would suggest that, if SetHistAdd() is called, then it would set the default hist file name in the fContext. Or alternatively, a second parameter in the SetHistAdd function in order to set a custom hist file name.

The function to set the hist name is:
Gl_histinit

image

@bellenot
Copy link
Member

bellenot commented Feb 17, 2021

Yes, I know, but since this is quite a high level change, I don't think I will introduce it in the TGCommandPlugin class. You should do that in your own application. For example, you can simply copy and paste what is done in TRint.cxx:

   // Goto into raw terminal input mode
   char defhist[kMAXPATHLEN];
   snprintf(defhist, sizeof(defhist), "%s/.root_hist", gSystem->HomeDirectory());
   logon = gEnv->GetValue("Rint.History", defhist);
   // In the code we had HistorySize and HistorySave, in the rootrc and doc
   // we have HistSize and HistSave. Keep the doc as it is and check
   // now also for HistSize and HistSave in case the user did not use
   // the History versions
   int hist_size = gEnv->GetValue("Rint.HistorySize", 500);
   if (hist_size == 500)
      hist_size = gEnv->GetValue("Rint.HistSize", 500);
   int hist_save = gEnv->GetValue("Rint.HistorySave", 400);
   if (hist_save == 400)
      hist_save = gEnv->GetValue("Rint.HistSave", 400);
   const char *envHist = gSystem->Getenv("ROOT_HIST");
   if (envHist) {
      hist_size = atoi(envHist);
      envHist = strchr(envHist, ':');
      if (envHist)
         hist_save = atoi(envHist+1);
   }
   Gl_histsize(hist_size, hist_save);
   Gl_histinit((char *)logon);

You can even specify your own history file, if you don't want to use the default.

@ferdymercury
Copy link
Collaborator Author

Thank you very much Bertrand for the explanation. I tried with this one-liner in my code, and now it works perfectly:

Gl_histinit(gEnv->GetValue("Rint.History", gSystem->HomeDirectory()));

The problem I see if it is not introduced in the plugin is that the SetHistAdd() function has no effect. To avoid confusion, I would suggest to add the following in the documentation:

// The function SetHistAdd() is needed for a standalone TApplication to log the TGCommandPlugin commands into a ROOT history file.
// However, this function has no effect if the user does not explictly set on his standalone application the name of the ROOT history file.
// To log into the default ROOT history file, call this on the user-side of the code:
// Gl_histinit(gEnv->GetValue("Rint.History", gSystem->HomeDirectory()));
// Otherwise, replace the argument of Gl_histinit with a text file name you want to use for application-specific logging.

After that addition, it looks great and ready to merge in my opinion :)

@bellenot
Copy link
Member

Right, I agree, I will add those lines in the doc. Thanks!

@bellenot bellenot added this to Issues in Fixed in 6.24/00 via automation Feb 25, 2021
nicknagi pushed a commit to nicknagi/root that referenced this issue Mar 30, 2021
…t-project#7220)

* Add up and down arrow keys handling (fix root-project#7180)

* Improve the command plugin behaviour

 - Add <tab> completion (thanks @ferdymercury)
 - Improve the `up` and `down` key handling
 - Add a temporary string to save the current user input
 - Update copyright

* Document the `SetHistAdd()` method

* Doxygen formatting (thanks Olivier)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

3 participants