Re: [patch 2/2] MdeModulePkg/Browser: Share default if some default value are not specified

["Gao, Liming" <liming.gao@intel.com>]

Dandan:
  I have two comments. 
1) Why define EFI_HII_MAX_SUPPORT_DEFAULT_TYPE as 8?
2) Could we generate DefaultStoreListHead as ascending order? If so, don't need call GetDefaultIdArray() function for each default retrieve. 

Thanks
Liming
> -----Original Message-----
> From: Bi, Dandan
> Sent: Friday, August 05, 2016 10:13 AM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [patch 2/2] MdeModulePkg/Browser: Share default if some default
> value are not specified
> 
> Add a new implementation policy of getting default in SetupBrowser.
> The new policy is only for the situation that a question has default
> value but doesn't have default value for all supported default type.
> In this case, we will choose the smallest default id from the existing
> defaults, and share its value to other default id which has no
> default value.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> Reviewed-by: Eric Dong <eric.dong@intel.com>
> ---
>  MdeModulePkg/Universal/SetupBrowserDxe/Setup.c | 66
> +++++++++++++++++++++++++-
>  MdeModulePkg/Universal/SetupBrowserDxe/Setup.h |  5 ++
>  2 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
> b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
> index 6b38547..9473eb1 100644
> --- a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
> +++ b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
> @@ -4016,10 +4016,50 @@ ValueToOption (
>    }
> 
>    return NULL;
>  }
> 
> +/**
> +  This function is to get the default Id array, the default ids are in ascending
> order in the array.
> +
> +  @param  FormSet                    The form set.
> +  @param  DefaultIdArray              Point to the default Id Array.
> +
> +  @retval DefaultIdCount             Return the default Id number in the array.
> +
> +**/
> +UINT16
> +GetDefaultIdArray (
> +  IN FORM_BROWSER_FORMSET             *FormSet,
> +  IN OUT UINT16                       *DefaultIdArray
> +  )
> +{
> +  FORMSET_DEFAULTSTORE            *DefaultStore;
> +  LIST_ENTRY                      *DefaultLink;
> +  UINT16                          DefaultIdCount;
> +  UINT16                          Index;
> +
> +  DefaultIdCount = 0;
> +
> +  DefaultLink = GetFirstNode (&FormSet->DefaultStoreListHead);
> +  while (!IsNull (&FormSet->DefaultStoreListHead, DefaultLink)) {
> +    DefaultStore = FORMSET_DEFAULTSTORE_FROM_LINK(DefaultLink);
> +    Index = DefaultIdCount;
> +    //
> +    // Insert the DefaultId to the Array with ascending order.
> +    //
> +    while (Index > 0  && DefaultStore->DefaultId < DefaultIdArray[Index - 1])
> {
> +      DefaultIdArray[Index] = DefaultIdArray[Index -1];
> +      Index--;
> +    }
> +    DefaultIdArray[Index] = DefaultStore->DefaultId;
> +    DefaultLink = GetNextNode (&FormSet->DefaultStoreListHead,
> DefaultLink);
> +    DefaultIdCount++;
> +  }
> +
> +  return DefaultIdCount;
> +}
> 
>  /**
>    Reset Question to its default value.
> 
>    @param  FormSet                The form set.
> @@ -4048,29 +4088,38 @@ GetQuestionDefault (
>    EFI_HII_CONFIG_ACCESS_PROTOCOL  *ConfigAccess;
>    EFI_BROWSER_ACTION_REQUEST      ActionRequest;
>    INTN                            Action;
>    CHAR16                          *NewString;
>    EFI_IFR_TYPE_VALUE              *TypeValue;
> +  UINT16                          DefaultIdNumber;
> +  UINT16
> DefaultIdArray[EFI_HII_MAX_SUPPORT_DEFAULT_TYPE];
> +  UINT16                          ReGetCount;
> +  UINT16                          OriginalDefaultId;
> 
>    Status   = EFI_NOT_FOUND;
>    StrValue = NULL;
> +  ReGetCount  = 0;
> +  OriginalDefaultId  = DefaultId;
> 
>    //
>    // Statement don't have storage, skip them
>    //
>    if (Question->QuestionId == 0) {
>      return Status;
>    }
> 
> +  DefaultIdNumber = GetDefaultIdArray (FormSet, DefaultIdArray);
> +
>    //
>    // There are Five ways to specify default value for a Question:
>    //  1, use call back function (highest priority)
>    //  2, use ExtractConfig function
>    //  3, use nested EFI_IFR_DEFAULT
>    //  4, set flags of EFI_ONE_OF_OPTION (provide Standard and
> Manufacturing default)
>    //  5, set flags of EFI_IFR_CHECKBOX (provide Standard and Manufacturing
> default) (lowest priority)
>    //
> +ReGetDefault:
>    HiiValue = &Question->HiiValue;
>    TypeValue = &HiiValue->Value;
>    if (HiiValue->Type == EFI_IFR_TYPE_BUFFER) {
>      //
>      // For orderedlist, need to pass the BufferValue to Callback function.
> @@ -4233,11 +4282,26 @@ GetQuestionDefault (
>        return EFI_SUCCESS;
>      }
>    }
> 
>    //
> -  // For Questions without default
> +  // For question without default value for current default Id, we try to re-
> get the default value form other default id in the DefaultIdArray.
> +  // If get, will exit the function, if not, will choose next default id in the
> DefaultIdArray.
> +  // The default id in DefaultIdArray are in ascending order to make sure
> choose the smallest default id every time.
> +  //
> +  while (ReGetCount < DefaultIdNumber) {
> +    DefaultId = DefaultIdArray[ReGetCount];
> +    if (DefaultId == OriginalDefaultId) {
> +      ReGetCount ++;
> +      continue;
> +    }
> +    ReGetCount ++;
> +    goto ReGetDefault;
> +  }
> +
> +  //
> +  // For Questions without default value for all the default id in the
> DefaultIdArray.
>    //
>    Status = EFI_NOT_FOUND;
>    switch (Question->Operand) {
>    case EFI_IFR_NUMERIC_OP:
>      //
> diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.h
> b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.h
> index cbc5401..21f392c 100644
> --- a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.h
> +++ b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.h
> @@ -61,10 +61,15 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #define UI_ACTION_REFRESH_FORM       1
>  #define UI_ACTION_REFRESH_FORMSET    2
>  #define UI_ACTION_EXIT               3
> 
>  //
> +// The maximum number of default type
> +//
> +#define EFI_HII_MAX_SUPPORT_DEFAULT_TYPE  0x08
> +
> +//
>  //
>  // Time definitions
>  //
>  #define ONE_SECOND  10000000
> 
> --
> 1.9.5.msysgit.1