[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D106192#2893862 <https://reviews.llvm.org/D106192#2893862>, @OmarEmaraDev wrote:

> I am currently breaking this patch into smaller independent viable patches as suggested.

That will make reviews much easier. Are you going to land the other smaller diffs first and then update this one after hey have landed?



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1309
+    FileSpec file_spec(m_content);
+    FileSystem::Instance().Resolve(file_spec);
+    return file_spec;
----------------
OmarEmaraDev wrote:
> clayborg wrote:
> > It should be a property of the FileFieldDelegate if the file is for the current system. We might be specifying a file on a remote system, so it doesn't always have to exist, nor should it be resolved on the current system. When creating a target we would always want to specify this file should exist on the current system. 
> > 
> > 
> I was trying to add properties that indicates if the file is local or not and if it should be resolved or not. But they all seems identical to the need_to_exist property. If m_need_to_exist is false, then the file will not be resolved and will not be checked with the host file system (Which is what we want to remote files). If it is true, then it will be resolved and checked with the host file system (Which is what we want for local files). So adding more properties might be redundant? What do you think?
That sounds fine. No need to add more fields that don't make sense


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1315
   bool m_need_to_exist;
+  bool m_required;
 };
----------------
OmarEmaraDev wrote:
> clayborg wrote:
> > Should this to into the FieldDelegate so we don't need to put it into both FileFieldDelegate and DirectoryFieldDelegate? Then the default FieldDelegate::FieldDelegateExitCallback() function could be:
> > 
> > ```
> > virtual void FieldDelegate::FieldDelegateExitCallback() {
> >   if (!m_required)
> >     return;
> >   // Check value with another virtual function?
> >   FieldDelegateValueIsValid();
> > }
> > ```
> > This seems like many fields would want to require being set and we don't want to re-invent this for each kind of field.
> The fields that can have a required property is the TextField and its derivatives (File, Directory, and Number), so we can can add this logic there. This is implemented in D106458.
Sounds good!


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2609
+
+    if (!FileSystem::Instance().Exists(file_spec)) {
+      SetError("File doesn't exist!");
----------------
OmarEmaraDev wrote:
> clayborg wrote:
> > Won't this be taken care of already by the File field? We should construct the FileDelegate so that:
> > - the file must exist
> > - the file gets resolved on the local system
> > - the file field is required
> > This way, the only way for the user to get to the "Create" button if is if they already were able to set a valid and required by the FileFieldDelegate class.
> D106459 implements the necessary bits to allow for this.
Sounds good!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106192/new/

https://reviews.llvm.org/D106192