Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic

[Dong, Eric]

Reviewed-by: Eric Dong <eric.d...@intel.com>

> -Original Message-
> From: Bi, Dandan
> Sent: Monday, October 17, 2016 5:08 PM
> To: Laszlo Ersek; edk2-de...@ml01.01.org
> Cc: Dong, Eric; Gao, Liming
> Subject: RE: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the 
> codes logic
> 
> Hi Laszlo,
> 
> Thank you very much for  your comments!
> I have split this patch into 5 independent patches with following subject :
> [patch 1/5] MdeModulePkg/BMMUI: ...
> [patch 2/5] MdeModulePkg/BMMUI: ...
> [patch 3/5] MdeModulePkg/BMMUI: ...
> [patch 4/5] MdeModulePkg/BMMUI: ...
> [patch 5/5] MdeModulePkg/BMMUI: ...
> 
> Hi Liming/Eric
> Please review the new patches and  ignore this one.  Sorry for any 
> inconvenience.
> 
> 
> Regards,
> Dandan
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, October 14, 2016 8:34 PM
> To: Bi, Dandan <dandan...@intel.com>; edk2-de...@ml01.01.org
> Cc: Dong, Eric <eric.d...@intel.com>; Gao, Liming <liming@intel.com>
> Subject: Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the 
> codes logic
> 
> On 10/14/16 08:43, Dandan Bi wrote:
> > This patch is mainly to:
> > 1. Enhance the error handling codes when set variable fail.
> > 2. Enhance the logic to fix some incorrect UI behaviors.
> 
> My apologies, but both the subject line and the commit message are mostly 
> impenetrable.
> 
> This patch should be split up into a series of two patches, minimally 
> (according to the two goals above that it implements), and each
> change should be described correctly in both the subject line and in the 
> commit message.
> 
> If I got a bug report for OVMF that I managed to bisect back to this patch, 
> I'd be *completely* helpless figuring out what it does.
> 
> What kind of variables are set by the code? What happens now if setting those 
> variables fails? What is the expected behavior instead that
> the
> (first) patch implements?
> 
> What are those incorrect UI behaviors? When do they happen? What does the 
> second patch do to address those issues?
> 
> Dear Developers, please *stop* writing subject lines like
> 
>   "Enhance the code in DNS driver"
>   "Enhance the codes logic"
> 
> those subject lines are *completely* useless. You could replace all those 
> subject lines, without any loss of information, with the following
> one:
> 
>   "Do Work"
> 
> Please spend time thinking about the granularity, the focus of your patches, 
> as a *standalone activity* during development. Ask yourselves,
> "Is this patch small enough? Am I doing two or more independent things here? 
> Is the subject line clear enough? If a person sees the code
> for the first time, will my commit message help them?"
> 
> You don't write the commit message for yourselves only, you write it for 
> other developers who might have absolutely no clue what's going
> on in your module.
> 
> Thanks
> Laszlo
> 
> > V2: Update the Console/Terminal menu when the related question changed.
> >
> > Cc: Liming Gao <liming@intel.com>
> > Cc: Eric Dong <eric.d...@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Dandan Bi <dandan...@intel.com>
> > ---
> >  .../BootMaintenanceManagerUiLib/BootMaintenance.c  | 390 
> > -
> >  .../BootMaintenanceManagerUiLib/UpdatePage.c   |  42 ++-
> >  .../Library/BootMaintenanceManagerUiLib/Variable.c |  28 +-
> >  3 files changed, 357 insertions(+), 103 deletions(-)
> >
> > diff --git
> > a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c
> > b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c
> > index a190596..924eb49 100644
> > ---
> > a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c
> > +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance
> > +++ .c
> > @@ -442,10 +442,197 @@ BmmExtractDevicePathFromHiiHandle (
> >return ConvertDevicePathToText(DevicePathFromHandle (DriverHandle),
> > FALSE, FALSE);
> >
> >  }
> >
> >  /**
> > +  Converts the unicode character of the string from uppercase to lowercase.
> > +  This is a internal function.
> > +
> > +  @param ConfigString  String to be converted
> > +
> > +**/
> > +VOID
> > +HiiToLower (
> > +  IN EFI_STRING  ConfigString
> > +  )
> > +{
> > +  EFI_STRING  String;
> > +  BOOLEAN Lower;
> > +
> > +  ASSERT (ConfigString != NULL);

Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic

[Laszlo Ersek]

On 10/17/16 11:07, Bi, Dandan wrote:
> Hi Laszlo,
> 
> Thank you very much for  your comments!  
> I have split this patch into 5 independent patches with following subject : 
> [patch 1/5] MdeModulePkg/BMMUI: ...
> [patch 2/5] MdeModulePkg/BMMUI: ...
> [patch 3/5] MdeModulePkg/BMMUI: ...
> [patch 4/5] MdeModulePkg/BMMUI: ...
> [patch 5/5] MdeModulePkg/BMMUI: ...

Thank you very much -- the new subjects look much-much better. Also, I
think it's a nice trick to shorten "Boot Maintenance Manager UI" as
"BMMUI", it's understandable and it saves quite a few characters in the
subjects.

Thanks!
Laszlo

> Hi Liming/Eric
> Please review the new patches and  ignore this one.  Sorry for any 
> inconvenience.
> 
> 
> Regards,
> Dandan
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Friday, October 14, 2016 8:34 PM
> To: Bi, Dandan <dandan...@intel.com>; edk2-de...@ml01.01.org
> Cc: Dong, Eric <eric.d...@intel.com>; Gao, Liming <liming....@intel.com>
> Subject: Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the 
> codes logic
> 
> On 10/14/16 08:43, Dandan Bi wrote:
>> This patch is mainly to:
>> 1. Enhance the error handling codes when set variable fail.
>> 2. Enhance the logic to fix some incorrect UI behaviors.
> 
> My apologies, but both the subject line and the commit message are mostly 
> impenetrable.
> 
> This patch should be split up into a series of two patches, minimally 
> (according to the two goals above that it implements), and each change should 
> be described correctly in both the subject line and in the commit message.
> 
> If I got a bug report for OVMF that I managed to bisect back to this patch, 
> I'd be *completely* helpless figuring out what it does.
> 
> What kind of variables are set by the code? What happens now if setting those 
> variables fails? What is the expected behavior instead that the
> (first) patch implements?
> 
> What are those incorrect UI behaviors? When do they happen? What does the 
> second patch do to address those issues?
> 
> Dear Developers, please *stop* writing subject lines like
> 
>   "Enhance the code in DNS driver"
>   "Enhance the codes logic"
> 
> those subject lines are *completely* useless. You could replace all those 
> subject lines, without any loss of information, with the following
> one:
> 
>   "Do Work"
> 
> Please spend time thinking about the granularity, the focus of your patches, 
> as a *standalone activity* during development. Ask yourselves, "Is this patch 
> small enough? Am I doing two or more independent things here? Is the subject 
> line clear enough? If a person sees the code for the first time, will my 
> commit message help them?"
> 
> You don't write the commit message for yourselves only, you write it for 
> other developers who might have absolutely no clue what's going on in your 
> module.
> 
> Thanks
> Laszlo
> 
>> V2: Update the Console/Terminal menu when the related question changed.
>>
>> Cc: Liming Gao <liming@intel.com>
>> Cc: Eric Dong <eric.d...@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Dandan Bi <dandan...@intel.com>
>> ---
>>  .../BootMaintenanceManagerUiLib/BootMaintenance.c  | 390 
>> -
>>  .../BootMaintenanceManagerUiLib/UpdatePage.c   |  42 ++-
>>  .../Library/BootMaintenanceManagerUiLib/Variable.c |  28 +-
>>  3 files changed, 357 insertions(+), 103 deletions(-)
>>
>> diff --git 
>> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c 
>> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c
>> index a190596..924eb49 100644
>> --- 
>> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c
>> +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance
>> +++ .c
>> @@ -442,10 +442,197 @@ BmmExtractDevicePathFromHiiHandle (
>>return ConvertDevicePathToText(DevicePathFromHandle (DriverHandle), 
>> FALSE, FALSE);
>>  
>>  }
>>  
>>  /**
>> +  Converts the unicode character of the string from uppercase to lowercase.
>> +  This is a internal function.
>> +
>> +  @param ConfigString  String to be converted
>> +
>> +**/
>> +VOID
>> +HiiToLower (
>> +  IN EFI_STRING  ConfigString
>> +  )
>> +{
>> +  EFI_STRING  String;
>> +  BOOLEAN Lower;
>> +
>> +  ASSERT (ConfigString != NULL);
>> +
>> +  //
>> +  // Convert all hex digits 

Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic

[Laszlo Ersek]

On 10/14/16 08:43, Dandan Bi wrote:
> This patch is mainly to:
> 1. Enhance the error handling codes when set variable fail.
> 2. Enhance the logic to fix some incorrect UI behaviors.

My apologies, but both the subject line and the commit message are
mostly impenetrable.

This patch should be split up into a series of two patches, minimally
(according to the two goals above that it implements), and each change
should be described correctly in both the subject line and in the commit
message.

If I got a bug report for OVMF that I managed to bisect back to this
patch, I'd be *completely* helpless figuring out what it does.

What kind of variables are set by the code? What happens now if setting
those variables fails? What is the expected behavior instead that the
(first) patch implements?

What are those incorrect UI behaviors? When do they happen? What does
the second patch do to address those issues?

Dear Developers, please *stop* writing subject lines like

  "Enhance the code in DNS driver"
  "Enhance the codes logic"

those subject lines are *completely* useless. You could replace all
those subject lines, without any loss of information, with the following
one:

  "Do Work"

Please spend time thinking about the granularity, the focus of your
patches, as a *standalone activity* during development. Ask yourselves,
"Is this patch small enough? Am I doing two or more independent things
here? Is the subject line clear enough? If a person sees the code for
the first time, will my commit message help them?"

You don't write the commit message for yourselves only, you write it for
other developers who might have absolutely no clue what's going on in
your module.

Thanks
Laszlo

> V2: Update the Console/Terminal menu when the related question changed.
> 
> Cc: Liming Gao 
> Cc: Eric Dong 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi 
> ---
>  .../BootMaintenanceManagerUiLib/BootMaintenance.c  | 390 
> -
>  .../BootMaintenanceManagerUiLib/UpdatePage.c   |  42 ++-
>  .../Library/BootMaintenanceManagerUiLib/Variable.c |  28 +-
>  3 files changed, 357 insertions(+), 103 deletions(-)
> 
> diff --git 
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c 
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c
> index a190596..924eb49 100644
> --- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c
> +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c
> @@ -442,10 +442,197 @@ BmmExtractDevicePathFromHiiHandle (
>return ConvertDevicePathToText(DevicePathFromHandle (DriverHandle), FALSE, 
> FALSE);
>  
>  }
>  
>  /**
> +  Converts the unicode character of the string from uppercase to lowercase.
> +  This is a internal function.
> +
> +  @param ConfigString  String to be converted
> +
> +**/
> +VOID
> +HiiToLower (
> +  IN EFI_STRING  ConfigString
> +  )
> +{
> +  EFI_STRING  String;
> +  BOOLEAN Lower;
> +
> +  ASSERT (ConfigString != NULL);
> +
> +  //
> +  // Convert all hex digits in range [A-F] in the configuration header to 
> [a-f]
> +  //
> +  for (String = ConfigString, Lower = FALSE; *String != L'\0'; String++) {
> +if (*String == L'=') {
> +  Lower = TRUE;
> +} else if (*String == L'&') {
> +  Lower = FALSE;
> +} else if (Lower && *String >= L'A' && *String <= L'F') {
> +  *String = (CHAR16) (*String - L'A' + L'a');
> +}
> +  }
> +}
> +
> +/**
> +  Update the progress string through the offset value.
> +
> +  @param Offset   The offset value
> +  @param ConfigurationPoint to the configuration string.
> +
> +**/
> +EFI_STRING
> +UpdateProgress(
> +  IN  UINTN   Offset,
> +  IN  EFI_STRING  Configuration
> +)
> +{
> +  UINTN   Length;
> +  EFI_STRING  StringPtr;
> +  EFI_STRING  ReturnString;
> +
> +  StringPtr= NULL;
> +  ReturnString = NULL;
> +
> +  //
> +  // = followed by a Null-terminator.
> +  // Length = StrLen (L"=") + 4 + 1
> +  //
> +  Length= StrLen (L"=") + 4 + 1;
> +
> +  StringPtr = AllocateZeroPool (Length * sizeof (CHAR16));
> +
> +  if (StringPtr == NULL) {
> +return  NULL;
> +  }
> +
> +  UnicodeSPrint (
> +StringPtr,
> +(8 + 4 + 1) * sizeof (CHAR16),
> +L"=%04x",
> +Offset
> +);
> +
> +  ReturnString = StrStr (Configuration, StringPtr);
> +
> +  if (ReturnString == NULL) {
> +//
> +// If doesn't find the string in Configuration, convert the string to 
> lower case then search again.
> +//
> +HiiToLower (StringPtr);
> +ReturnString = StrStr (Configuration, StringPtr);
> +  }
> +
> +  FreePool (StringPtr);
> +
> +  return ReturnString;
> +}
> +
> +/**
> +  Update the terminal content in TerminalMenu.
> +
> +  @param BmmData   The BMM fake NV data.
> +
> +**/
> +VOID
> +UpdateTerminalContent (
> +  IN BMM_FAKE_NV_DATA   *BmmData
> +  )
> +{
> +  UINT16