[RFC] [MdePkg] UefiLib: CreatePopUp

[RFC] [MdePkg] UefiLib: CreatePopUp

[Felix Poludov]

UefiLib library class (MdePkg ) includes CreatePopUp function.
The function displays a message box.
There is certainly more than one way to draw a message box.
If homogenous user interface is a project requirement,
CreatePopUp is likely to be overridden to align message box appearance with the platform look and feel.
The function can be overridden by creating a project specific UefiLib instance, but this seems like an overkill
because the rest of the UefiLib, which is quite big, would have to be duplicated.

One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
I suggest changing CreatePopUp implementation to delegate pop up drawing to a new
function UiCreatePopUp provided by a new library class UiLib.h.

I would like to solicit feedback for this proposal.
If there will be no major objections, I'll start working on a patch.

Thanks
Felix

Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Dong, Eric]

Hi Felix,

Do you want to provide a new solution for CreatePopup or just want to split CreatePopup from UefiLib?  We already has a proposal to provide new API to replace CreatePopup. This new API will use modal form to paint the UI. Detail you can see the proposal in below link:
https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20CreatePopup%20API.pptx

Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Felix Poludov
> Sent: Friday, October 28, 2016 5:12 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> UefiLib library class (MdePkg ) includes CreatePopUp function.
> The function displays a message box.
> There is certainly more than one way to draw a message box.
> If homogenous user interface is a project requirement,
> CreatePopUp is likely to be overridden to align message box appearance with the platform look and feel.
> The function can be overridden by creating a project specific UefiLib instance, but this seems like an overkill
> because the rest of the UefiLib, which is quite big, would have to be duplicated.
> 
> One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> I suggest changing CreatePopUp implementation to delegate pop up drawing to a new
> function UiCreatePopUp provided by a new library class UiLib.h.
> 
> I would like to solicit feedback for this proposal.
> If there will be no major objections, I'll start working on a patch.
> 
> Thanks
> Felix
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Felix Poludov]

Hi Eric,

My goal is to facilitate CreatePopUp customization.
Since UI is one of the most customizable areas in the firmware projects, an ability to easily replace UI element would be useful.
Thank you for providing the presentation.
I agree with the problem statement. It describes some of the reasons behind my request.
As far as the solution you propose, you are introducing a new function HiiGetUserSelection, which is more powerful, but still implements a specific look-and-feel.
So it should be possible to easily replace HiiGetUserSelection with a project specific version to align implementation with project-specific UI.
Which library class are you planning to add HiiGetUserSelection to?
Another question is, what are you planning to do with the existing CreatePopUp function?
If you just remove it, existing projects that use the function will break.
With my proposal, CreatePopUp can be easily replaced by picking a different library instance.
For example, we can have a legacy instance that implements current behavior as well as and advanced instance that implements popup using HII infrastructure.

Thanks
Felix

-----Original Message-----
From: Dong, Eric [mailto:eric.dong@intel.com]
Sent: Thursday, October 27, 2016 10:36 PM
To: Felix Poludov; edk2-devel@lists.01.org
Cc: Gao, Liming; Bi, Dandan
Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp

Hi Felix,

Do you want to provide a new solution for CreatePopup or just want to split CreatePopup from UefiLib?  We already has a proposal to provide new API to replace CreatePopup. This new API will use modal form to paint the UI. Detail you can see the proposal in below link:
https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20CreatePopup%20API.pptx

Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Felix Poludov
> Sent: Friday, October 28, 2016 5:12 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
>
> UefiLib library class (MdePkg ) includes CreatePopUp function.
> The function displays a message box.
> There is certainly more than one way to draw a message box.
> If homogenous user interface is a project requirement,
> CreatePopUp is likely to be overridden to align message box appearance with the platform look and feel.
> The function can be overridden by creating a project specific UefiLib instance, but this seems like an overkill
> because the rest of the UefiLib, which is quite big, would have to be duplicated.
>
> One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> I suggest changing CreatePopUp implementation to delegate pop up drawing to a new
> function UiCreatePopUp provided by a new library class UiLib.h.
>
> I would like to solicit feedback for this proposal.
> If there will be no major objections, I'll start working on a patch.
>
> Thanks
> Felix
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Dong, Eric]

Hi Felix,

Add my comments below.

> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Friday, October 28, 2016 9:52 PM
> To: Dong, Eric; edk2-devel@lists.01.org
> Cc: Gao, Liming; Bi, Dandan
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Hi Eric,
> 
> My goal is to facilitate CreatePopUp customization.
> Since UI is one of the most customizable areas in the firmware projects, an ability to easily replace UI element would be useful.
> Thank you for providing the presentation.
> I agree with the problem statement. It describes some of the reasons behind my request.
> As far as the solution you propose, you are introducing a new function HiiGetUserSelection, which is more powerful, but still implements a
> specific look-and-feel.
> So it should be possible to easily replace HiiGetUserSelection with a project specific version to align implementation with project-specific
> UI.
> Which library class are you planning to add HiiGetUserSelection to?
[[Eric]] I plan to add this API to HiiLib which existed at MdeModulePkg/Library/UefiHiiLib. I think it's belong to HII scope.

> Another question is, what are you planning to do with the existing CreatePopUp function?
[[Eric]] I plan to not change it. Just suggest user to use new API and deprecated it later.

> If you just remove it, existing projects that use the function will break.
> With my proposal, CreatePopUp can be easily replaced by picking a different library instance.
> For example, we can have a legacy instance that implements current behavior as well as and advanced instance that implements popup
> using HII infrastructure.
> 
> Thanks
> Felix
> 
> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Thursday, October 27, 2016 10:36 PM
> To: Felix Poludov; edk2-devel@lists.01.org
> Cc: Gao, Liming; Bi, Dandan
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Hi Felix,
> 
> Do you want to provide a new solution for CreatePopup or just want to split CreatePopup from UefiLib?  We already has a proposal to
> provide new API to replace CreatePopup. This new API will use modal form to paint the UI. Detail you can see the proposal in below link:
> https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20CreatePopup%20API.pptx
> 
> Thanks,
> Eric
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Felix Poludov
> > Sent: Friday, October 28, 2016 5:12 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > UefiLib library class (MdePkg ) includes CreatePopUp function.
> > The function displays a message box.
> > There is certainly more than one way to draw a message box.
> > If homogenous user interface is a project requirement,
> > CreatePopUp is likely to be overridden to align message box appearance with the platform look and feel.
> > The function can be overridden by creating a project specific UefiLib instance, but this seems like an overkill
> > because the rest of the UefiLib, which is quite big, would have to be duplicated.
> >
> > One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> > I suggest changing CreatePopUp implementation to delegate pop up drawing to a new
> > function UiCreatePopUp provided by a new library class UiLib.h.
> >
> > I would like to solicit feedback for this proposal.
> > If there will be no major objections, I'll start working on a patch.
> >
> > Thanks
> > Felix
> >
> > Please consider the environment before printing this email.
> >
> > The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> > intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> > intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> > sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Felix Poludov]

Eric,

1. If you are not changing CretePopUp, your proposal does not really solve my problem.
It means my proposal still has merit.
Do you have problem with moving CreatePopUp implementation into a new library class?
Once again, the function will stay in UefiLib, but the implementation will be changed to call a new function UiCreatePopUp
from the new library class UiLib.

2. In my opinion, adding HiiGetUserSelection to HiiLib is a bad idea because it will create the same problem as with CretePopUp.
The rest of HiiLib is generic. It operates within the scope of HII database definitions from UEFI specification.
The new HiiGetUserSelection you are proposing deals with presentation, which means it is may get changed to match project UI.
HiiGetUserSelection can be added to the same new UiLib that I'm proposing.

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
Sent: Monday, October 31, 2016 4:26 AM
To: Felix Poludov; edk2-devel@lists.01.org
Cc: Bi, Dandan; Gao, Liming
Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp

Hi Felix,

Add my comments below.

> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Friday, October 28, 2016 9:52 PM
> To: Dong, Eric; edk2-devel@lists.01.org
> Cc: Gao, Liming; Bi, Dandan
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Hi Eric,
>
> My goal is to facilitate CreatePopUp customization.
> Since UI is one of the most customizable areas in the firmware projects, an ability to easily replace UI element would be useful.
> Thank you for providing the presentation.
> I agree with the problem statement. It describes some of the reasons behind my request.
> As far as the solution you propose, you are introducing a new function HiiGetUserSelection, which is more powerful, but still implements a
> specific look-and-feel.
> So it should be possible to easily replace HiiGetUserSelection with a project specific version to align implementation with project-specific
> UI.
> Which library class are you planning to add HiiGetUserSelection to?
[[Eric]] I plan to add this API to HiiLib which existed at MdeModulePkg/Library/UefiHiiLib. I think it's belong to HII scope.

> Another question is, what are you planning to do with the existing CreatePopUp function?
[[Eric]] I plan to not change it. Just suggest user to use new API and deprecated it later.

> If you just remove it, existing projects that use the function will break.
> With my proposal, CreatePopUp can be easily replaced by picking a different library instance.
> For example, we can have a legacy instance that implements current behavior as well as and advanced instance that implements popup
> using HII infrastructure.
>
> Thanks
> Felix
>
> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Thursday, October 27, 2016 10:36 PM
> To: Felix Poludov; edk2-devel@lists.01.org
> Cc: Gao, Liming; Bi, Dandan
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Hi Felix,
>
> Do you want to provide a new solution for CreatePopup or just want to split CreatePopup from UefiLib?  We already has a proposal to
> provide new API to replace CreatePopup. This new API will use modal form to paint the UI. Detail you can see the proposal in below link:
> https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20CreatePopup%20API.pptx
>
> Thanks,
> Eric
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Felix Poludov
> > Sent: Friday, October 28, 2016 5:12 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > UefiLib library class (MdePkg ) includes CreatePopUp function.
> > The function displays a message box.
> > There is certainly more than one way to draw a message box.
> > If homogenous user interface is a project requirement,
> > CreatePopUp is likely to be overridden to align message box appearance with the platform look and feel.
> > The function can be overridden by creating a project specific UefiLib instance, but this seems like an overkill
> > because the rest of the UefiLib, which is quite big, would have to be duplicated.
> >
> > One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> > I suggest changing CreatePopUp implementation to delegate pop up drawing to a new
> > function UiCreatePopUp provided by a new library class UiLib.h.
> >
> > I would like to solicit feedback for this proposal.
> > If there will be no major objections, I'll start working on a patch.
> >
> > Thanks
> > Felix
> >
> > Please consider the environment before printing this email.
> >
> > The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> > intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> > intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> > sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Dong, Eric]

Felix,

Add my comments below.

-----Original Message-----
From: Felix Poludov [mailto:Felixp@ami.com] 
Sent: Tuesday, November 1, 2016 4:22 AM
To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp

Eric,

1. If you are not changing CretePopUp, your proposal does not really solve my problem.
It means my proposal still has merit.
Do you have problem with moving CreatePopUp implementation into a new library class?
Once again, the function will stay in UefiLib, but the implementation will be changed to call a new function UiCreatePopUp from the new library class UiLib.
[[Eric]] My proposal bases on modal form to show the popup dialog.  The modal form
is painted by the browser. So the UI will changed in different browser. I think you can 
update your browser to show the different UI.
I not prefer your solution because this is an incompatible change and we must avoid it.
It will impact a lot of core codes which use CreatePopup API.


2. In my opinion, adding HiiGetUserSelection to HiiLib is a bad idea because it will create the same problem as with CretePopUp.
The rest of HiiLib is generic. It operates within the scope of HII database definitions from UEFI specification.
The new HiiGetUserSelection you are proposing deals with presentation, which means it is may get changed to match project UI.
HiiGetUserSelection can be added to the same new UiLib that I'm proposing.
[[Eric]] My proposal bases on modal form defined in UEFI Spec. Modal form UI decides 
by the browser. Base on this reason, so I put this new API to the HiiLib. I only add one 
new API and related to HII, so I prefer not add new library class.

I think you want to split the API to new library just want to reduce the override code size? Or other reason? 

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
Sent: Monday, October 31, 2016 4:26 AM
To: Felix Poludov; edk2-devel@lists.01.org
Cc: Bi, Dandan; Gao, Liming
Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp

Hi Felix,

Add my comments below.

> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Friday, October 28, 2016 9:52 PM
> To: Dong, Eric; edk2-devel@lists.01.org
> Cc: Gao, Liming; Bi, Dandan
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Hi Eric,
> 
> My goal is to facilitate CreatePopUp customization.
> Since UI is one of the most customizable areas in the firmware projects, an ability to easily replace UI element would be useful.
> Thank you for providing the presentation.
> I agree with the problem statement. It describes some of the reasons behind my request.
> As far as the solution you propose, you are introducing a new function 
> HiiGetUserSelection, which is more powerful, but still implements a specific look-and-feel.
> So it should be possible to easily replace HiiGetUserSelection with a 
> project specific version to align implementation with project-specific UI.
> Which library class are you planning to add HiiGetUserSelection to?
[[Eric]] I plan to add this API to HiiLib which existed at MdeModulePkg/Library/UefiHiiLib. I think it's belong to HII scope.

> Another question is, what are you planning to do with the existing CreatePopUp function?
[[Eric]] I plan to not change it. Just suggest user to use new API and deprecated it later.

> If you just remove it, existing projects that use the function will break.
> With my proposal, CreatePopUp can be easily replaced by picking a different library instance.
> For example, we can have a legacy instance that implements current 
> behavior as well as and advanced instance that implements popup using HII infrastructure.
> 
> Thanks
> Felix
> 
> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Thursday, October 27, 2016 10:36 PM
> To: Felix Poludov; edk2-devel@lists.01.org
> Cc: Gao, Liming; Bi, Dandan
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Hi Felix,
> 
> Do you want to provide a new solution for CreatePopup or just want to 
> split CreatePopup from UefiLib?  We already has a proposal to provide new API to replace CreatePopup. This new API will use modal form to paint the UI. Detail you can see the proposal in below link:
> https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20
> CreatePopup%20API.pptx
> 
> Thanks,
> Eric
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 
> > Of Felix Poludov
> > Sent: Friday, October 28, 2016 5:12 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > UefiLib library class (MdePkg ) includes CreatePopUp function.
> > The function displays a message box.
> > There is certainly more than one way to draw a message box.
> > If homogenous user interface is a project requirement, CreatePopUp 
> > is likely to be overridden to align message box appearance with the platform look and feel.
> > The function can be overridden by creating a project specific 
> > UefiLib instance, but this seems like an overkill because the rest of the UefiLib, which is quite big, would have to be duplicated.
> >
> > One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> > I suggest changing CreatePopUp implementation to delegate pop up 
> > drawing to a new function UiCreatePopUp provided by a new library class UiLib.h.
> >
> > I would like to solicit feedback for this proposal.
> > If there will be no major objections, I'll start working on a patch.
> >
> > Thanks
> > Felix
> >
> > Please consider the environment before printing this email.
> >
> > The information contained in this message may be confidential and 
> > proprietary to American Megatrends, Inc.  This communication is 
> > intended to be read only by the individual or entity to whom it is 
> > addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and 
> proprietary to American Megatrends, Inc.  This communication is 
> intended to be read only by the individual or entity to whom it is 
> addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Felix Poludov]

Eric,

See my comments below

-----Original Message-----
From: Dong, Eric [mailto:eric.dong@intel.com]
Sent: Tuesday, November 01, 2016 2:55 AM
To: Felix Poludov; edk2-devel@lists.01.org
Cc: Bi, Dandan; Gao, Liming
Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp

Felix,

Add my comments below.

-----Original Message-----
From: Felix Poludov [mailto:Felixp@ami.com]
Sent: Tuesday, November 1, 2016 4:22 AM
To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp

Eric,

1. If you are not changing CretePopUp, your proposal does not really solve my problem.
It means my proposal still has merit.
Do you have problem with moving CreatePopUp implementation into a new library class?
Once again, the function will stay in UefiLib, but the implementation will be changed to call a new function UiCreatePopUp from the new library class UiLib.
[[Eric]] My proposal bases on modal form to show the popup dialog.  The modal form
is painted by the browser. So the UI will changed in different browser. I think you can
update your browser to show the different UI.
I not prefer your solution because this is an incompatible change and we must avoid it.
It will impact a lot of core codes which use CreatePopup API.

[[[Felix]]] Since you have no plans to change CreatePopUp implementation, existing code that uses CreatePopUp will like to keep using it, which means
popup box will keep looking "strange".
As far as my proposal, I'm not sure why you are saying that it's backward compatible.
Let me explain one more time, what I propose:
1. No changes to UefiLib.h (CreatePopUp is still there)
2.  A single line change in [LibraryClasses] section of MdePkg/Library/UefiLib/UefiLib.inf. New library class UiLib is added there.
3.  CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is replaced with:
VOID
EFIAPI
CreatePopUp (
  IN  UINTN          Attribute,
  OUT EFI_INPUT_KEY  *Key,      OPTIONAL
  ...
  )
{
  VA_LIST                          Args;
  VA_START (Args, Key);
  UiCreatePopUp(Attribute,Ket,Args);
}
4. New library class UiLib.h is added. New library instance for the class is added. The library contains UiCreatePopUp function.
Function implementation is copied from original CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c.

To summarize:
- There is no need to change code that consumes CreatePopUp because the function is still there
- CreatePopUp consumers see no difference in look-and-feel and behavior because default CreatePopUp implementation is the same as the current one.
- Project owners that strive unified UI can override UiLib library class.
I agree that creating a new library class for a single function does not feel right, but we may have other functions that deal with presentation in the future.
See continuation of my answer below...

2. In my opinion, adding HiiGetUserSelection to HiiLib is a bad idea because it will create the same problem as with CretePopUp.
The rest of HiiLib is generic. It operates within the scope of HII database definitions from UEFI specification.
The new HiiGetUserSelection you are proposing deals with presentation, which means it is may get changed to match project UI.
HiiGetUserSelection can be added to the same new UiLib that I'm proposing.
[[Eric]] My proposal bases on modal form defined in UEFI Spec. Modal form UI decides
by the browser. Base on this reason, so I put this new API to the HiiLib. I only add one
new API and related to HII, so I prefer not add new library class.

I think you want to split the API to new library just want to reduce the override code size? Or other reason?
[[[Felix]]] 1. I agree, if new function is based on standard HII Modal form, HiiLib is a good place for it.
2. Yes, the main reason is a clean project porting.
Today I have two options:
Option 1: replace CreatePopUp implementation in Console.c with my custom implementation
Option 2: create custom instance of UefiLib
With both options, I'm deviating from standard EDKII code base, which means I will have to merge my changes whenever I want to upgrade EDKII tree.
Also I will have to manually migrate the patch from project to project.


-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
Sent: Monday, October 31, 2016 4:26 AM
To: Felix Poludov; edk2-devel@lists.01.org
Cc: Bi, Dandan; Gao, Liming
Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp

Hi Felix,

Add my comments below.

> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Friday, October 28, 2016 9:52 PM
> To: Dong, Eric; edk2-devel@lists.01.org
> Cc: Gao, Liming; Bi, Dandan
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Hi Eric,
>
> My goal is to facilitate CreatePopUp customization.
> Since UI is one of the most customizable areas in the firmware projects, an ability to easily replace UI element would be useful.
> Thank you for providing the presentation.
> I agree with the problem statement. It describes some of the reasons behind my request.
> As far as the solution you propose, you are introducing a new function
> HiiGetUserSelection, which is more powerful, but still implements a specific look-and-feel.
> So it should be possible to easily replace HiiGetUserSelection with a
> project specific version to align implementation with project-specific UI.
> Which library class are you planning to add HiiGetUserSelection to?
[[Eric]] I plan to add this API to HiiLib which existed at MdeModulePkg/Library/UefiHiiLib. I think it's belong to HII scope.

> Another question is, what are you planning to do with the existing CreatePopUp function?
[[Eric]] I plan to not change it. Just suggest user to use new API and deprecated it later.

> If you just remove it, existing projects that use the function will break.
> With my proposal, CreatePopUp can be easily replaced by picking a different library instance.
> For example, we can have a legacy instance that implements current
> behavior as well as and advanced instance that implements popup using HII infrastructure.
>
> Thanks
> Felix
>
> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Thursday, October 27, 2016 10:36 PM
> To: Felix Poludov; edk2-devel@lists.01.org
> Cc: Gao, Liming; Bi, Dandan
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Hi Felix,
>
> Do you want to provide a new solution for CreatePopup or just want to
> split CreatePopup from UefiLib?  We already has a proposal to provide new API to replace CreatePopup. This new API will use modal form to paint the UI. Detail you can see the proposal in below link:
> https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20
> CreatePopup%20API.pptx
>
> Thanks,
> Eric
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > Of Felix Poludov
> > Sent: Friday, October 28, 2016 5:12 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > UefiLib library class (MdePkg ) includes CreatePopUp function.
> > The function displays a message box.
> > There is certainly more than one way to draw a message box.
> > If homogenous user interface is a project requirement, CreatePopUp
> > is likely to be overridden to align message box appearance with the platform look and feel.
> > The function can be overridden by creating a project specific
> > UefiLib instance, but this seems like an overkill because the rest of the UefiLib, which is quite big, would have to be duplicated.
> >
> > One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> > I suggest changing CreatePopUp implementation to delegate pop up
> > drawing to a new function UiCreatePopUp provided by a new library class UiLib.h.
> >
> > I would like to solicit feedback for this proposal.
> > If there will be no major objections, I'll start working on a patch.
> >
> > Thanks
> > Felix
> >
> > Please consider the environment before printing this email.
> >
> > The information contained in this message may be confidential and
> > proprietary to American Megatrends, Inc.  This communication is
> > intended to be read only by the individual or entity to whom it is
> > addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and
> proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is
> addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Dong, Eric]

Hi Felix,

Thanks for your clearly explanation, I misunderstand your changes before.  But I think this change still has small impact because the platforms DSC file need to include the new UiLib instance. 

I think the new UiLib  library class need to be added into MdePkg because UefiLib need to depend on it. This may need get some input from MdePkg maintainer. If you add this new UiLib, I will add my new API into this library class.

Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Felix Poludov
> Sent: Thursday, November 03, 2016 7:13 AM
> To: Dong, Eric; edk2-devel@lists.01.org
> Cc: Bi, Dandan; Gao, Liming
> Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Eric,
> 
> See my comments below
> 
> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Tuesday, November 01, 2016 2:55 AM
> To: Felix Poludov; edk2-devel@lists.01.org
> Cc: Bi, Dandan; Gao, Liming
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Felix,
> 
> Add my comments below.
> 
> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Tuesday, November 1, 2016 4:22 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Eric,
> 
> 1. If you are not changing CretePopUp, your proposal does not really solve my problem.
> It means my proposal still has merit.
> Do you have problem with moving CreatePopUp implementation into a new library class?
> Once again, the function will stay in UefiLib, but the implementation will be changed to call a new function UiCreatePopUp from the new
> library class UiLib.
> [[Eric]] My proposal bases on modal form to show the popup dialog.  The modal form
> is painted by the browser. So the UI will changed in different browser. I think you can
> update your browser to show the different UI.
> I not prefer your solution because this is an incompatible change and we must avoid it.
> It will impact a lot of core codes which use CreatePopup API.
> 
> [[[Felix]]] Since you have no plans to change CreatePopUp implementation, existing code that uses CreatePopUp will like to keep using it,
> which means
> popup box will keep looking "strange".
> As far as my proposal, I'm not sure why you are saying that it's backward compatible.
> Let me explain one more time, what I propose:
> 1. No changes to UefiLib.h (CreatePopUp is still there)
> 2.  A single line change in [LibraryClasses] section of MdePkg/Library/UefiLib/UefiLib.inf. New library class UiLib is added there.
> 3.  CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is replaced with:
> VOID
> EFIAPI
> CreatePopUp (
>   IN  UINTN          Attribute,
>   OUT EFI_INPUT_KEY  *Key,      OPTIONAL
>   ...
>   )
> {
>   VA_LIST                          Args;
>   VA_START (Args, Key);
>   UiCreatePopUp(Attribute,Ket,Args);
> }
> 4. New library class UiLib.h is added. New library instance for the class is added. The library contains UiCreatePopUp function.
> Function implementation is copied from original CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c.
> 
> To summarize:
> - There is no need to change code that consumes CreatePopUp because the function is still there
> - CreatePopUp consumers see no difference in look-and-feel and behavior because default CreatePopUp implementation is the same as
> the current one.
> - Project owners that strive unified UI can override UiLib library class.
> I agree that creating a new library class for a single function does not feel right, but we may have other functions that deal with
> presentation in the future.
> See continuation of my answer below...
> 
> 2. In my opinion, adding HiiGetUserSelection to HiiLib is a bad idea because it will create the same problem as with CretePopUp.
> The rest of HiiLib is generic. It operates within the scope of HII database definitions from UEFI specification.
> The new HiiGetUserSelection you are proposing deals with presentation, which means it is may get changed to match project UI.
> HiiGetUserSelection can be added to the same new UiLib that I'm proposing.
> [[Eric]] My proposal bases on modal form defined in UEFI Spec. Modal form UI decides
> by the browser. Base on this reason, so I put this new API to the HiiLib. I only add one
> new API and related to HII, so I prefer not add new library class.
> 
> I think you want to split the API to new library just want to reduce the override code size? Or other reason?
> [[[Felix]]] 1. I agree, if new function is based on standard HII Modal form, HiiLib is a good place for it.
> 2. Yes, the main reason is a clean project porting.
> Today I have two options:
> Option 1: replace CreatePopUp implementation in Console.c with my custom implementation
> Option 2: create custom instance of UefiLib
> With both options, I'm deviating from standard EDKII code base, which means I will have to merge my changes whenever I want to
> upgrade EDKII tree.
> Also I will have to manually migrate the patch from project to project.
> 
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
> Sent: Monday, October 31, 2016 4:26 AM
> To: Felix Poludov; edk2-devel@lists.01.org
> Cc: Bi, Dandan; Gao, Liming
> Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Hi Felix,
> 
> Add my comments below.
> 
> > -----Original Message-----
> > From: Felix Poludov [mailto:Felixp@ami.com]
> > Sent: Friday, October 28, 2016 9:52 PM
> > To: Dong, Eric; edk2-devel@lists.01.org
> > Cc: Gao, Liming; Bi, Dandan
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Hi Eric,
> >
> > My goal is to facilitate CreatePopUp customization.
> > Since UI is one of the most customizable areas in the firmware projects, an ability to easily replace UI element would be useful.
> > Thank you for providing the presentation.
> > I agree with the problem statement. It describes some of the reasons behind my request.
> > As far as the solution you propose, you are introducing a new function
> > HiiGetUserSelection, which is more powerful, but still implements a specific look-and-feel.
> > So it should be possible to easily replace HiiGetUserSelection with a
> > project specific version to align implementation with project-specific UI.
> > Which library class are you planning to add HiiGetUserSelection to?
> [[Eric]] I plan to add this API to HiiLib which existed at MdeModulePkg/Library/UefiHiiLib. I think it's belong to HII scope.
> 
> > Another question is, what are you planning to do with the existing CreatePopUp function?
> [[Eric]] I plan to not change it. Just suggest user to use new API and deprecated it later.
> 
> > If you just remove it, existing projects that use the function will break.
> > With my proposal, CreatePopUp can be easily replaced by picking a different library instance.
> > For example, we can have a legacy instance that implements current
> > behavior as well as and advanced instance that implements popup using HII infrastructure.
> >
> > Thanks
> > Felix
> >
> > -----Original Message-----
> > From: Dong, Eric [mailto:eric.dong@intel.com]
> > Sent: Thursday, October 27, 2016 10:36 PM
> > To: Felix Poludov; edk2-devel@lists.01.org
> > Cc: Gao, Liming; Bi, Dandan
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Hi Felix,
> >
> > Do you want to provide a new solution for CreatePopup or just want to
> > split CreatePopup from UefiLib?  We already has a proposal to provide new API to replace CreatePopup. This new API will use modal
> form to paint the UI. Detail you can see the proposal in below link:
> > https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20
> > CreatePopup%20API.pptx
> >
> > Thanks,
> > Eric
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Felix Poludov
> > > Sent: Friday, October 28, 2016 5:12 AM
> > > To: edk2-devel@lists.01.org
> > > Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> > >
> > > UefiLib library class (MdePkg ) includes CreatePopUp function.
> > > The function displays a message box.
> > > There is certainly more than one way to draw a message box.
> > > If homogenous user interface is a project requirement, CreatePopUp
> > > is likely to be overridden to align message box appearance with the platform look and feel.
> > > The function can be overridden by creating a project specific
> > > UefiLib instance, but this seems like an overkill because the rest of the UefiLib, which is quite big, would have to be duplicated.
> > >
> > > One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> > > I suggest changing CreatePopUp implementation to delegate pop up
> > > drawing to a new function UiCreatePopUp provided by a new library class UiLib.h.
> > >
> > > I would like to solicit feedback for this proposal.
> > > If there will be no major objections, I'll start working on a patch.
> > >
> > > Thanks
> > > Felix
> > >
> > > Please consider the environment before printing this email.
> > >
> > > The information contained in this message may be confidential and
> > > proprietary to American Megatrends, Inc.  This communication is
> > > intended to be read only by the individual or entity to whom it is
> > > addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of
> this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and
> then delete or destroy all copies of the transmission.
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> >
> > Please consider the environment before printing this email.
> >
> > The information contained in this message may be confidential and
> > proprietary to American Megatrends, Inc.  This communication is
> > intended to be read only by the individual or entity to whom it is
> > addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this
> message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then
> delete or destroy all copies of the transmission.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Yao, Jiewen]

Hi
I have another idea to resolve the MdePkg dependency issue.

We can create a new UiLib in MdeModulePkg with interface such as: UiLibCreatePopUp().
Then we update all the *caller* to use the new interface. I did a search and find caller below:

We can add DEPRECATE flag for the UefiLib:CreatePopUp(), to avoid it being used any more.

C:\home\Edk-II\MdeModulePkg\Application\UiApp\FrontPage.c(1156):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, StringBuffer1, StringBuffer2, NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(324):        CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(346):      CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(378):      CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(404):        CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(431):      CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\DriverSampleDxe\DriverSample.c(1407):          CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\DriverSampleDxe\DriverSample.c(1830):            CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\DriverSampleDxe\DriverSample.c(1872):            CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\DriverSampleDxe\DriverSample.c(1901):          CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(613):      CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid IP address!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(619):      CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Subnet Mask!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(625):      CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Gateway!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(634):          CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Dns Server!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(641):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Dns Server!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1150):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid IP address!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1158):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Subnet Mask!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1166):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Gateway!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1177):            CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Dns Server!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1184):          CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Dns Server!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(101):    CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(721):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid iSCSI Name!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(731):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid IP address!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(743):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Subnet Mask!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(755):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Gateway!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(767):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid IP address!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(779):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid iSCSI Name!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(797):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid LUN string!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(855):              CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Gateway address is set but subnet mask is zero.", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(859):              CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Local IP and Gateway are not in the same subnet.", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(871):            CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Target IP is invalid!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(882):            CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(896):            CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"CHAP Name or CHAP Secret is invalid!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(904):            CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Reverse CHAP Name or Reverse CHAP Secret is invalid!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\PlatformDriOverrideDxe\PlatDriOverrideDxe.c(1434):          CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Single Override Info too large, Saving Error!", NULL);
  C:\home\Edk-II\NetworkPkg\HttpBootDxe\HttpBootConfig.c(495):      CreatePopUp (
  C:\home\Edk-II\NetworkPkg\Ip6Dxe\Ip6ConfigNv.c(1801):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\Ip6Dxe\Ip6ConfigNv.c(1818):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\Ip6Dxe\Ip6ConfigNv.c(1835):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\Ip6Dxe\Ip6ConfigNv.c(1852):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(212):    CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(514):      CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(535):          CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(544):          CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(560):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(575):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(591):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(604):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(648):      CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2205):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2250):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2353):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2370):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2387):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2404):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2421):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2444):        CreatePopUp (

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
Sent: Thursday, November 3, 2016 3:33 PM
To: Felix Poludov <Felixp@ami.com>; edk2-devel@lists.01.org
Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp

Hi Felix,

Thanks for your clearly explanation, I misunderstand your changes before.  But I think this change still has small impact because the platforms DSC file need to include the new UiLib instance.

I think the new UiLib  library class need to be added into MdePkg because UefiLib need to depend on it. This may need get some input from MdePkg maintainer. If you add this new UiLib, I will add my new API into this library class.

Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Felix Poludov
> Sent: Thursday, November 03, 2016 7:13 AM
> To: Dong, Eric; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Bi, Dandan; Gao, Liming
> Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Eric,
>
> See my comments below
>
> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Tuesday, November 01, 2016 2:55 AM
> To: Felix Poludov; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Bi, Dandan; Gao, Liming
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Felix,
>
> Add my comments below.
>
> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Tuesday, November 1, 2016 4:22 AM
> To: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Eric,
>
> 1. If you are not changing CretePopUp, your proposal does not really solve my problem.
> It means my proposal still has merit.
> Do you have problem with moving CreatePopUp implementation into a new library class?
> Once again, the function will stay in UefiLib, but the implementation will be changed to call a new function UiCreatePopUp from the new
> library class UiLib.
> [[Eric]] My proposal bases on modal form to show the popup dialog.  The modal form
> is painted by the browser. So the UI will changed in different browser. I think you can
> update your browser to show the different UI.
> I not prefer your solution because this is an incompatible change and we must avoid it.
> It will impact a lot of core codes which use CreatePopup API.
>
> [[[Felix]]] Since you have no plans to change CreatePopUp implementation, existing code that uses CreatePopUp will like to keep using it,
> which means
> popup box will keep looking "strange".
> As far as my proposal, I'm not sure why you are saying that it's backward compatible.
> Let me explain one more time, what I propose:
> 1. No changes to UefiLib.h (CreatePopUp is still there)
> 2.  A single line change in [LibraryClasses] section of MdePkg/Library/UefiLib/UefiLib.inf. New library class UiLib is added there.
> 3.  CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is replaced with:
> VOID
> EFIAPI
> CreatePopUp (
>   IN  UINTN          Attribute,
>   OUT EFI_INPUT_KEY  *Key,      OPTIONAL
>   ...
>   )
> {
>   VA_LIST                          Args;
>   VA_START (Args, Key);
>   UiCreatePopUp(Attribute,Ket,Args);
> }
> 4. New library class UiLib.h is added. New library instance for the class is added. The library contains UiCreatePopUp function.
> Function implementation is copied from original CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c.
>
> To summarize:
> - There is no need to change code that consumes CreatePopUp because the function is still there
> - CreatePopUp consumers see no difference in look-and-feel and behavior because default CreatePopUp implementation is the same as
> the current one.
> - Project owners that strive unified UI can override UiLib library class.
> I agree that creating a new library class for a single function does not feel right, but we may have other functions that deal with
> presentation in the future.
> See continuation of my answer below...
>
> 2. In my opinion, adding HiiGetUserSelection to HiiLib is a bad idea because it will create the same problem as with CretePopUp.
> The rest of HiiLib is generic. It operates within the scope of HII database definitions from UEFI specification.
> The new HiiGetUserSelection you are proposing deals with presentation, which means it is may get changed to match project UI.
> HiiGetUserSelection can be added to the same new UiLib that I'm proposing.
> [[Eric]] My proposal bases on modal form defined in UEFI Spec. Modal form UI decides
> by the browser. Base on this reason, so I put this new API to the HiiLib. I only add one
> new API and related to HII, so I prefer not add new library class.
>
> I think you want to split the API to new library just want to reduce the override code size? Or other reason?
> [[[Felix]]] 1. I agree, if new function is based on standard HII Modal form, HiiLib is a good place for it.
> 2. Yes, the main reason is a clean project porting.
> Today I have two options:
> Option 1: replace CreatePopUp implementation in Console.c with my custom implementation
> Option 2: create custom instance of UefiLib
> With both options, I'm deviating from standard EDKII code base, which means I will have to merge my changes whenever I want to
> upgrade EDKII tree.
> Also I will have to manually migrate the patch from project to project.
>
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
> Sent: Monday, October 31, 2016 4:26 AM
> To: Felix Poludov; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Bi, Dandan; Gao, Liming
> Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Hi Felix,
>
> Add my comments below.
>
> > -----Original Message-----
> > From: Felix Poludov [mailto:Felixp@ami.com]
> > Sent: Friday, October 28, 2016 9:52 PM
> > To: Dong, Eric; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > Cc: Gao, Liming; Bi, Dandan
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Hi Eric,
> >
> > My goal is to facilitate CreatePopUp customization.
> > Since UI is one of the most customizable areas in the firmware projects, an ability to easily replace UI element would be useful.
> > Thank you for providing the presentation.
> > I agree with the problem statement. It describes some of the reasons behind my request.
> > As far as the solution you propose, you are introducing a new function
> > HiiGetUserSelection, which is more powerful, but still implements a specific look-and-feel.
> > So it should be possible to easily replace HiiGetUserSelection with a
> > project specific version to align implementation with project-specific UI.
> > Which library class are you planning to add HiiGetUserSelection to?
> [[Eric]] I plan to add this API to HiiLib which existed at MdeModulePkg/Library/UefiHiiLib. I think it's belong to HII scope.
>
> > Another question is, what are you planning to do with the existing CreatePopUp function?
> [[Eric]] I plan to not change it. Just suggest user to use new API and deprecated it later.
>
> > If you just remove it, existing projects that use the function will break.
> > With my proposal, CreatePopUp can be easily replaced by picking a different library instance.
> > For example, we can have a legacy instance that implements current
> > behavior as well as and advanced instance that implements popup using HII infrastructure.
> >
> > Thanks
> > Felix
> >
> > -----Original Message-----
> > From: Dong, Eric [mailto:eric.dong@intel.com]
> > Sent: Thursday, October 27, 2016 10:36 PM
> > To: Felix Poludov; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > Cc: Gao, Liming; Bi, Dandan
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Hi Felix,
> >
> > Do you want to provide a new solution for CreatePopup or just want to
> > split CreatePopup from UefiLib?  We already has a proposal to provide new API to replace CreatePopup. This new API will use modal
> form to paint the UI. Detail you can see the proposal in below link:
> > https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20
> > CreatePopup%20API.pptx
> >
> > Thanks,
> > Eric
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Felix Poludov
> > > Sent: Friday, October 28, 2016 5:12 AM
> > > To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > > Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> > >
> > > UefiLib library class (MdePkg ) includes CreatePopUp function.
> > > The function displays a message box.
> > > There is certainly more than one way to draw a message box.
> > > If homogenous user interface is a project requirement, CreatePopUp
> > > is likely to be overridden to align message box appearance with the platform look and feel.
> > > The function can be overridden by creating a project specific
> > > UefiLib instance, but this seems like an overkill because the rest of the UefiLib, which is quite big, would have to be duplicated.
> > >
> > > One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> > > I suggest changing CreatePopUp implementation to delegate pop up
> > > drawing to a new function UiCreatePopUp provided by a new library class UiLib.h.
> > >
> > > I would like to solicit feedback for this proposal.
> > > If there will be no major objections, I'll start working on a patch.
> > >
> > > Thanks
> > > Felix
> > >
> > > Please consider the environment before printing this email.
> > >
> > > The information contained in this message may be confidential and
> > > proprietary to American Megatrends, Inc.  This communication is
> > > intended to be read only by the individual or entity to whom it is
> > > addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of
> this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and
> then delete or destroy all copies of the transmission.
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> >
> > Please consider the environment before printing this email.
> >
> > The information contained in this message may be confidential and
> > proprietary to American Megatrends, Inc.  This communication is
> > intended to be read only by the individual or entity to whom it is
> > addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this
> message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then
> delete or destroy all copies of the transmission.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the
> intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the
> sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Felix Poludov]

Jiewen,

You suggestion is also an option, but I was hoping to achieve the goal without changing all the consumers.

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Thursday, November 03, 2016 7:25 PM
To: Dong, Eric; Felix Poludov; edk2-devel@lists.01.org
Cc: Bi, Dandan; Gao, Liming
Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp

Hi
I have another idea to resolve the MdePkg dependency issue.

We can create a new UiLib in MdeModulePkg with interface such as: UiLibCreatePopUp().
Then we update all the *caller* to use the new interface. I did a search and find caller below:

We can add DEPRECATE flag for the UefiLib:CreatePopUp(), to avoid it being used any more.

C:\home\Edk-II\MdeModulePkg\Application\UiApp\FrontPage.c(1156):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, StringBuffer1, StringBuffer2, NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(324):        CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(346):      CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(378):      CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(404):        CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Disk\RamDiskDxe\RamDiskImpl.c(431):      CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\DriverSampleDxe\DriverSample.c(1407):          CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\DriverSampleDxe\DriverSample.c(1830):            CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\DriverSampleDxe\DriverSample.c(1872):            CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\DriverSampleDxe\DriverSample.c(1901):          CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(613):      CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid IP address!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(619):      CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Subnet Mask!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(625):      CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Gateway!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(634):          CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Dns Server!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(641):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Dns Server!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1150):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid IP address!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1158):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Subnet Mask!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1166):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Gateway!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1177):            CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Dns Server!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\Ip4Dxe\Ip4Config2Nv.c(1184):          CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Dns Server!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(101):    CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(721):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid iSCSI Name!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(731):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid IP address!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(743):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Subnet Mask!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(755):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid Gateway!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(767):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid IP address!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(779):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid iSCSI Name!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(797):        CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Invalid LUN string!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(855):              CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Gateway address is set but subnet mask is zero.", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(859):              CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Local IP and Gateway are not in the same subnet.", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(871):            CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Target IP is invalid!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(882):            CreatePopUp (
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(896):            CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"CHAP Name or CHAP Secret is invalid!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\Network\IScsiDxe\IScsiConfig.c(904):            CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Reverse CHAP Name or Reverse CHAP Secret is invalid!", NULL);
  C:\home\Edk-II\MdeModulePkg\Universal\PlatformDriOverrideDxe\PlatDriOverrideDxe.c(1434):          CreatePopUp (EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, &Key, L"Single Override Info too large, Saving Error!", NULL);
  C:\home\Edk-II\NetworkPkg\HttpBootDxe\HttpBootConfig.c(495):      CreatePopUp (
  C:\home\Edk-II\NetworkPkg\Ip6Dxe\Ip6ConfigNv.c(1801):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\Ip6Dxe\Ip6ConfigNv.c(1818):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\Ip6Dxe\Ip6ConfigNv.c(1835):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\Ip6Dxe\Ip6ConfigNv.c(1852):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(212):    CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(514):      CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(535):          CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(544):          CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(560):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(575):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(591):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(604):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(648):      CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2205):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2250):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2353):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2370):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2387):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2404):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2421):        CreatePopUp (
  C:\home\Edk-II\NetworkPkg\IScsiDxe\IScsiConfig.c(2444):        CreatePopUp (

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
Sent: Thursday, November 3, 2016 3:33 PM
To: Felix Poludov <Felixp@ami.com>; edk2-devel@lists.01.org
Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp

Hi Felix,

Thanks for your clearly explanation, I misunderstand your changes before.  But I think this change still has small impact because the platforms DSC file need to include the new UiLib instance.

I think the new UiLib  library class need to be added into MdePkg because UefiLib need to depend on it. This may need get some input from MdePkg maintainer. If you add this new UiLib, I will add my new API into this library class.

Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Felix Poludov
> Sent: Thursday, November 03, 2016 7:13 AM
> To: Dong, Eric;
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Bi, Dandan; Gao, Liming
> Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Eric,
>
> See my comments below
>
> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Tuesday, November 01, 2016 2:55 AM
> To: Felix Poludov;
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Bi, Dandan; Gao, Liming
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Felix,
>
> Add my comments below.
>
> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Tuesday, November 1, 2016 4:22 AM
> To: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>;
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; Gao,
> Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Eric,
>
> 1. If you are not changing CretePopUp, your proposal does not really solve my problem.
> It means my proposal still has merit.
> Do you have problem with moving CreatePopUp implementation into a new library class?
> Once again, the function will stay in UefiLib, but the implementation
> will be changed to call a new function UiCreatePopUp from the new library class UiLib.
> [[Eric]] My proposal bases on modal form to show the popup dialog.
> The modal form is painted by the browser. So the UI will changed in
> different browser. I think you can update your browser to show the different UI.
> I not prefer your solution because this is an incompatible change and we must avoid it.
> It will impact a lot of core codes which use CreatePopup API.
>
> [[[Felix]]] Since you have no plans to change CreatePopUp
> implementation, existing code that uses CreatePopUp will like to keep
> using it, which means popup box will keep looking "strange".
> As far as my proposal, I'm not sure why you are saying that it's backward compatible.
> Let me explain one more time, what I propose:
> 1. No changes to UefiLib.h (CreatePopUp is still there) 2.  A single
> line change in [LibraryClasses] section of MdePkg/Library/UefiLib/UefiLib.inf. New library class UiLib is added there.
> 3.  CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is replaced with:
> VOID
> EFIAPI
> CreatePopUp (
>   IN  UINTN          Attribute,
>   OUT EFI_INPUT_KEY  *Key,      OPTIONAL
>   ...
>   )
> {
>   VA_LIST                          Args;
>   VA_START (Args, Key);
>   UiCreatePopUp(Attribute,Ket,Args);
> }
> 4. New library class UiLib.h is added. New library instance for the class is added. The library contains UiCreatePopUp function.
> Function implementation is copied from original CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c.
>
> To summarize:
> - There is no need to change code that consumes CreatePopUp because
> the function is still there
> - CreatePopUp consumers see no difference in look-and-feel and
> behavior because default CreatePopUp implementation is the same as the current one.
> - Project owners that strive unified UI can override UiLib library class.
> I agree that creating a new library class for a single function does
> not feel right, but we may have other functions that deal with presentation in the future.
> See continuation of my answer below...
>
> 2. In my opinion, adding HiiGetUserSelection to HiiLib is a bad idea because it will create the same problem as with CretePopUp.
> The rest of HiiLib is generic. It operates within the scope of HII database definitions from UEFI specification.
> The new HiiGetUserSelection you are proposing deals with presentation, which means it is may get changed to match project UI.
> HiiGetUserSelection can be added to the same new UiLib that I'm proposing.
> [[Eric]] My proposal bases on modal form defined in UEFI Spec. Modal
> form UI decides by the browser. Base on this reason, so I put this new
> API to the HiiLib. I only add one new API and related to HII, so I prefer not add new library class.
>
> I think you want to split the API to new library just want to reduce the override code size? Or other reason?
> [[[Felix]]] 1. I agree, if new function is based on standard HII Modal form, HiiLib is a good place for it.
> 2. Yes, the main reason is a clean project porting.
> Today I have two options:
> Option 1: replace CreatePopUp implementation in Console.c with my
> custom implementation Option 2: create custom instance of UefiLib With
> both options, I'm deviating from standard EDKII code base, which means
> I will have to merge my changes whenever I want to upgrade EDKII tree.
> Also I will have to manually migrate the patch from project to project.
>
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Dong, Eric
> Sent: Monday, October 31, 2016 4:26 AM
> To: Felix Poludov;
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Bi, Dandan; Gao, Liming
> Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Hi Felix,
>
> Add my comments below.
>
> > -----Original Message-----
> > From: Felix Poludov [mailto:Felixp@ami.com]
> > Sent: Friday, October 28, 2016 9:52 PM
> > To: Dong, Eric;
> > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > Cc: Gao, Liming; Bi, Dandan
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Hi Eric,
> >
> > My goal is to facilitate CreatePopUp customization.
> > Since UI is one of the most customizable areas in the firmware projects, an ability to easily replace UI element would be useful.
> > Thank you for providing the presentation.
> > I agree with the problem statement. It describes some of the reasons behind my request.
> > As far as the solution you propose, you are introducing a new
> > function HiiGetUserSelection, which is more powerful, but still implements a specific look-and-feel.
> > So it should be possible to easily replace HiiGetUserSelection with
> > a project specific version to align implementation with project-specific UI.
> > Which library class are you planning to add HiiGetUserSelection to?
> [[Eric]] I plan to add this API to HiiLib which existed at MdeModulePkg/Library/UefiHiiLib. I think it's belong to HII scope.
>
> > Another question is, what are you planning to do with the existing CreatePopUp function?
> [[Eric]] I plan to not change it. Just suggest user to use new API and deprecated it later.
>
> > If you just remove it, existing projects that use the function will break.
> > With my proposal, CreatePopUp can be easily replaced by picking a different library instance.
> > For example, we can have a legacy instance that implements current
> > behavior as well as and advanced instance that implements popup using HII infrastructure.
> >
> > Thanks
> > Felix
> >
> > -----Original Message-----
> > From: Dong, Eric [mailto:eric.dong@intel.com]
> > Sent: Thursday, October 27, 2016 10:36 PM
> > To: Felix Poludov;
> > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > Cc: Gao, Liming; Bi, Dandan
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Hi Felix,
> >
> > Do you want to provide a new solution for CreatePopup or just want
> > to split CreatePopup from UefiLib?  We already has a proposal to
> > provide new API to replace CreatePopup. This new API will use modal
> form to paint the UI. Detail you can see the proposal in below link:
> > https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%
> > 20
> > CreatePopup%20API.pptx
> >
> > Thanks,
> > Eric
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> > > Behalf Of Felix Poludov
> > > Sent: Friday, October 28, 2016 5:12 AM
> > > To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > > Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> > >
> > > UefiLib library class (MdePkg ) includes CreatePopUp function.
> > > The function displays a message box.
> > > There is certainly more than one way to draw a message box.
> > > If homogenous user interface is a project requirement, CreatePopUp
> > > is likely to be overridden to align message box appearance with the platform look and feel.
> > > The function can be overridden by creating a project specific
> > > UefiLib instance, but this seems like an overkill because the rest of the UefiLib, which is quite big, would have to be duplicated.
> > >
> > > One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> > > I suggest changing CreatePopUp implementation to delegate pop up
> > > drawing to a new function UiCreatePopUp provided by a new library class UiLib.h.
> > >
> > > I would like to solicit feedback for this proposal.
> > > If there will be no major objections, I'll start working on a patch.
> > >
> > > Thanks
> > > Felix
> > >
> > > Please consider the environment before printing this email.
> > >
> > > The information contained in this message may be confidential and
> > > proprietary to American Megatrends, Inc.  This communication is
> > > intended to be read only by the individual or entity to whom it is
> > > addressed or by their designee. If the reader of this message is
> > > not the intended recipient, you are on notice that any
> > > distribution of
> this message, in any form, is strictly prohibited.  Please promptly
> notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> >
> > Please consider the environment before printing this email.
> >
> > The information contained in this message may be confidential and
> > proprietary to American Megatrends, Inc.  This communication is
> > intended to be read only by the individual or entity to whom it is
> > addressed or by their designee. If the reader of this message is not
> > the intended recipient, you are on notice that any distribution of
> > this
> message, in any form, is strictly prohibited.  Please promptly notify
> the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and
> proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is
> addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and
> proprietary to American Megatrends, Inc.  This communication is
> intended to be read only by the individual or entity to whom it is
> addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Felix Poludov]

Mike,

Do you have an opinion?
Let me do a quick recap in case you didn't follow the thread.
I raised an issue with the CreatePopUp function (UefiLib library class).
The popup box produced by the function may not align with the project UI.
So the project owner may want to replace CreatePopUp implementation.
Today I have two options:
Option 1: replace CreatePopUp implementation in Console.c with my custom implementation
Option 2: create custom instance of UefiLib
Both options lead to project maintenance complications (EDKII code base upgrades and feature migration from project to project becomes more complicated)

I propose changing CreatePopUp implementation to delegate pop up
drawing to a new function UiCreatePopUp provided by a new library class UiLib.h.
More specifically:
1. No changes to UefiLib.h (CreatePopUp is still there)
2.  A single line change in [LibraryClasses] section of MdePkg/Library/UefiLib/UefiLib.inf. New library class UiLib is added there.
3.  CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is replaced with:
VOID
EFIAPI
CreatePopUp (
   IN  UINTN          Attribute,
   OUT EFI_INPUT_KEY  *Key,      OPTIONAL
   ...
   )
 {
   VA_LIST                          Args;
   VA_START (Args, Key);
   UiCreatePopUp(Attribute,Ket,Args);
 }
 4. New library class UiLib.h is added. New library instance for the class is added. The library contains UiCreatePopUp function.
 Function implementation is copied from original CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c.

Thanks
Felix

-----Original Message-----
From: Dong, Eric [mailto:eric.dong@intel.com]
Sent: Thursday, November 03, 2016 3:33 AM
To: Felix Poludov; edk2-devel@lists.01.org
Cc: Bi, Dandan; Gao, Liming
Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp

Hi Felix,

Thanks for your clearly explanation, I misunderstand your changes before.  But I think this change still has small impact because the platforms DSC file need to include the new UiLib instance.

I think the new UiLib  library class need to be added into MdePkg because UefiLib need to depend on it. This may need get some input from MdePkg maintainer. If you add this new UiLib, I will add my new API into this library class.

Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Felix Poludov
> Sent: Thursday, November 03, 2016 7:13 AM
> To: Dong, Eric; edk2-devel@lists.01.org
> Cc: Bi, Dandan; Gao, Liming
> Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Eric,
>
> See my comments below
>
> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Tuesday, November 01, 2016 2:55 AM
> To: Felix Poludov; edk2-devel@lists.01.org
> Cc: Bi, Dandan; Gao, Liming
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Felix,
>
> Add my comments below.
>
> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Tuesday, November 1, 2016 4:22 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Eric,
>
> 1. If you are not changing CretePopUp, your proposal does not really solve my problem.
> It means my proposal still has merit.
> Do you have problem with moving CreatePopUp implementation into a new library class?
> Once again, the function will stay in UefiLib, but the implementation will be changed to call a new function UiCreatePopUp from the new
> library class UiLib.
> [[Eric]] My proposal bases on modal form to show the popup dialog.  The modal form
> is painted by the browser. So the UI will changed in different browser. I think you can
> update your browser to show the different UI.
> I not prefer your solution because this is an incompatible change and we must avoid it.
> It will impact a lot of core codes which use CreatePopup API.
>
> [[[Felix]]] Since you have no plans to change CreatePopUp implementation, existing code that uses CreatePopUp will like to keep using it,
> which means
> popup box will keep looking "strange".
> As far as my proposal, I'm not sure why you are saying that it's backward compatible.
> Let me explain one more time, what I propose:
> 1. No changes to UefiLib.h (CreatePopUp is still there)
> 2.  A single line change in [LibraryClasses] section of MdePkg/Library/UefiLib/UefiLib.inf. New library class UiLib is added there.
> 3.  CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is replaced with:
> VOID
> EFIAPI
> CreatePopUp (
>   IN  UINTN          Attribute,
>   OUT EFI_INPUT_KEY  *Key,      OPTIONAL
>   ...
>   )
> {
>   VA_LIST                          Args;
>   VA_START (Args, Key);
>   UiCreatePopUp(Attribute,Ket,Args);
> }
> 4. New library class UiLib.h is added. New library instance for the class is added. The library contains UiCreatePopUp function.
> Function implementation is copied from original CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c.
>
> To summarize:
> - There is no need to change code that consumes CreatePopUp because the function is still there
> - CreatePopUp consumers see no difference in look-and-feel and behavior because default CreatePopUp implementation is the same as
> the current one.
> - Project owners that strive unified UI can override UiLib library class.
> I agree that creating a new library class for a single function does not feel right, but we may have other functions that deal with
> presentation in the future.
> See continuation of my answer below...
>
> 2. In my opinion, adding HiiGetUserSelection to HiiLib is a bad idea because it will create the same problem as with CretePopUp.
> The rest of HiiLib is generic. It operates within the scope of HII database definitions from UEFI specification.
> The new HiiGetUserSelection you are proposing deals with presentation, which means it is may get changed to match project UI.
> HiiGetUserSelection can be added to the same new UiLib that I'm proposing.
> [[Eric]] My proposal bases on modal form defined in UEFI Spec. Modal form UI decides
> by the browser. Base on this reason, so I put this new API to the HiiLib. I only add one
> new API and related to HII, so I prefer not add new library class.
>
> I think you want to split the API to new library just want to reduce the override code size? Or other reason?
> [[[Felix]]] 1. I agree, if new function is based on standard HII Modal form, HiiLib is a good place for it.
> 2. Yes, the main reason is a clean project porting.
> Today I have two options:
> Option 1: replace CreatePopUp implementation in Console.c with my custom implementation
> Option 2: create custom instance of UefiLib
> With both options, I'm deviating from standard EDKII code base, which means I will have to merge my changes whenever I want to
> upgrade EDKII tree.
> Also I will have to manually migrate the patch from project to project.
>
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
> Sent: Monday, October 31, 2016 4:26 AM
> To: Felix Poludov; edk2-devel@lists.01.org
> Cc: Bi, Dandan; Gao, Liming
> Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
>
> Hi Felix,
>
> Add my comments below.
>
> > -----Original Message-----
> > From: Felix Poludov [mailto:Felixp@ami.com]
> > Sent: Friday, October 28, 2016 9:52 PM
> > To: Dong, Eric; edk2-devel@lists.01.org
> > Cc: Gao, Liming; Bi, Dandan
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Hi Eric,
> >
> > My goal is to facilitate CreatePopUp customization.
> > Since UI is one of the most customizable areas in the firmware projects, an ability to easily replace UI element would be useful.
> > Thank you for providing the presentation.
> > I agree with the problem statement. It describes some of the reasons behind my request.
> > As far as the solution you propose, you are introducing a new function
> > HiiGetUserSelection, which is more powerful, but still implements a specific look-and-feel.
> > So it should be possible to easily replace HiiGetUserSelection with a
> > project specific version to align implementation with project-specific UI.
> > Which library class are you planning to add HiiGetUserSelection to?
> [[Eric]] I plan to add this API to HiiLib which existed at MdeModulePkg/Library/UefiHiiLib. I think it's belong to HII scope.
>
> > Another question is, what are you planning to do with the existing CreatePopUp function?
> [[Eric]] I plan to not change it. Just suggest user to use new API and deprecated it later.
>
> > If you just remove it, existing projects that use the function will break.
> > With my proposal, CreatePopUp can be easily replaced by picking a different library instance.
> > For example, we can have a legacy instance that implements current
> > behavior as well as and advanced instance that implements popup using HII infrastructure.
> >
> > Thanks
> > Felix
> >
> > -----Original Message-----
> > From: Dong, Eric [mailto:eric.dong@intel.com]
> > Sent: Thursday, October 27, 2016 10:36 PM
> > To: Felix Poludov; edk2-devel@lists.01.org
> > Cc: Gao, Liming; Bi, Dandan
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Hi Felix,
> >
> > Do you want to provide a new solution for CreatePopup or just want to
> > split CreatePopup from UefiLib?  We already has a proposal to provide new API to replace CreatePopup. This new API will use modal
> form to paint the UI. Detail you can see the proposal in below link:
> > https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20
> > CreatePopup%20API.pptx
> >
> > Thanks,
> > Eric
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Felix Poludov
> > > Sent: Friday, October 28, 2016 5:12 AM
> > > To: edk2-devel@lists.01.org
> > > Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> > >
> > > UefiLib library class (MdePkg ) includes CreatePopUp function.
> > > The function displays a message box.
> > > There is certainly more than one way to draw a message box.
> > > If homogenous user interface is a project requirement, CreatePopUp
> > > is likely to be overridden to align message box appearance with the platform look and feel.
> > > The function can be overridden by creating a project specific
> > > UefiLib instance, but this seems like an overkill because the rest of the UefiLib, which is quite big, would have to be duplicated.
> > >
> > > One way to solve the problem is to move CreatePopUp to a new library class, however, this may break existing projects.
> > > I suggest changing CreatePopUp implementation to delegate pop up
> > > drawing to a new function UiCreatePopUp provided by a new library class UiLib.h.
> > >
> > > I would like to solicit feedback for this proposal.
> > > If there will be no major objections, I'll start working on a patch.
> > >
> > > Thanks
> > > Felix
> > >

Please consider the environment before printing this email.

The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [RFC] [MdePkg] UefiLib: CreatePopUp

[Kinney, Michael D]

Felix,

>From the thread, your description is missing the update to every platform DSC file
to add the new UiLib class to instance mapping even if the platform does not want 
any new PopUp behavior.

That is not necessarily bad.  There are many changes in the past that have required
a DSC and/or FDF file update.  However, once you have a proposal that requires a
DSC file update, doing the lib layering you describe is not required.  You could 
define new lib and update all the code that uses the old API to use the new API.

What your proposal does is provide a way for all PopUp calls to use the new UiLib, which 
with a single DSC file change can change what these look like.  So from the perspective
of providing a way for all code to use a new PopUp style, what you propose makes a lot
of sense.  We just need to clearly document the DSC file change and a patch for this
would have to include DSC file updates for all EDK II packages that need it.

Best regards,

Mike


> -----Original Message-----
> From: Felix Poludov [mailto:Felixp@ami.com]
> Sent: Thursday, November 3, 2016 4:55 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Mike,
> 
> Do you have an opinion?
> Let me do a quick recap in case you didn't follow the thread.
> I raised an issue with the CreatePopUp function (UefiLib library class).
> The popup box produced by the function may not align with the project UI.
> So the project owner may want to replace CreatePopUp implementation.
> Today I have two options:
> Option 1: replace CreatePopUp implementation in Console.c with my custom
> implementation
> Option 2: create custom instance of UefiLib
> Both options lead to project maintenance complications (EDKII code base upgrades and
> feature migration from project to project becomes more complicated)
> 
> I propose changing CreatePopUp implementation to delegate pop up
> drawing to a new function UiCreatePopUp provided by a new library class UiLib.h.
> More specifically:
> 1. No changes to UefiLib.h (CreatePopUp is still there)
> 2.  A single line change in [LibraryClasses] section of
> MdePkg/Library/UefiLib/UefiLib.inf. New library class UiLib is added there.
> 3.  CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is replaced with:
> VOID
> EFIAPI
> CreatePopUp (
>    IN  UINTN          Attribute,
>    OUT EFI_INPUT_KEY  *Key,      OPTIONAL
>    ...
>    )
>  {
>    VA_LIST                          Args;
>    VA_START (Args, Key);
>    UiCreatePopUp(Attribute,Ket,Args);
>  }
>  4. New library class UiLib.h is added. New library instance for the class is added.
> The library contains UiCreatePopUp function.
>  Function implementation is copied from original CreatePopUp implementation in
> MdePkg/Library/UefiLib/Console.c.
> 
> Thanks
> Felix
> 
> -----Original Message-----
> From: Dong, Eric [mailto:eric.dong@intel.com]
> Sent: Thursday, November 03, 2016 3:33 AM
> To: Felix Poludov; edk2-devel@lists.01.org
> Cc: Bi, Dandan; Gao, Liming
> Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> 
> Hi Felix,
> 
> Thanks for your clearly explanation, I misunderstand your changes before.  But I
> think this change still has small impact because the platforms DSC file need to
> include the new UiLib instance.
> 
> I think the new UiLib  library class need to be added into MdePkg because UefiLib
> need to depend on it. This may need get some input from MdePkg maintainer. If you add
> this new UiLib, I will add my new API into this library class.
> 
> Thanks,
> Eric
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Felix
> Poludov
> > Sent: Thursday, November 03, 2016 7:13 AM
> > To: Dong, Eric; edk2-devel@lists.01.org
> > Cc: Bi, Dandan; Gao, Liming
> > Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Eric,
> >
> > See my comments below
> >
> > -----Original Message-----
> > From: Dong, Eric [mailto:eric.dong@intel.com]
> > Sent: Tuesday, November 01, 2016 2:55 AM
> > To: Felix Poludov; edk2-devel@lists.01.org
> > Cc: Bi, Dandan; Gao, Liming
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Felix,
> >
> > Add my comments below.
> >
> > -----Original Message-----
> > From: Felix Poludov [mailto:Felixp@ami.com]
> > Sent: Tuesday, November 1, 2016 4:22 AM
> > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> > Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Eric,
> >
> > 1. If you are not changing CretePopUp, your proposal does not really solve my
> problem.
> > It means my proposal still has merit.
> > Do you have problem with moving CreatePopUp implementation into a new library
> class?
> > Once again, the function will stay in UefiLib, but the implementation will be
> changed to call a new function UiCreatePopUp from the new
> > library class UiLib.
> > [[Eric]] My proposal bases on modal form to show the popup dialog.  The modal form
> > is painted by the browser. So the UI will changed in different browser. I think you
> can
> > update your browser to show the different UI.
> > I not prefer your solution because this is an incompatible change and we must avoid
> it.
> > It will impact a lot of core codes which use CreatePopup API.
> >
> > [[[Felix]]] Since you have no plans to change CreatePopUp implementation, existing
> code that uses CreatePopUp will like to keep using it,
> > which means
> > popup box will keep looking "strange".
> > As far as my proposal, I'm not sure why you are saying that it's backward
> compatible.
> > Let me explain one more time, what I propose:
> > 1. No changes to UefiLib.h (CreatePopUp is still there)
> > 2.  A single line change in [LibraryClasses] section of
> MdePkg/Library/UefiLib/UefiLib.inf. New library class UiLib is added there.
> > 3.  CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is replaced
> with:
> > VOID
> > EFIAPI
> > CreatePopUp (
> >   IN  UINTN          Attribute,
> >   OUT EFI_INPUT_KEY  *Key,      OPTIONAL
> >   ...
> >   )
> > {
> >   VA_LIST                          Args;
> >   VA_START (Args, Key);
> >   UiCreatePopUp(Attribute,Ket,Args);
> > }
> > 4. New library class UiLib.h is added. New library instance for the class is added.
> The library contains UiCreatePopUp function.
> > Function implementation is copied from original CreatePopUp implementation in
> MdePkg/Library/UefiLib/Console.c.
> >
> > To summarize:
> > - There is no need to change code that consumes CreatePopUp because the function is
> still there
> > - CreatePopUp consumers see no difference in look-and-feel and behavior because
> default CreatePopUp implementation is the same as
> > the current one.
> > - Project owners that strive unified UI can override UiLib library class.
> > I agree that creating a new library class for a single function does not feel
> right, but we may have other functions that deal with
> > presentation in the future.
> > See continuation of my answer below...
> >
> > 2. In my opinion, adding HiiGetUserSelection to HiiLib is a bad idea because it
> will create the same problem as with CretePopUp.
> > The rest of HiiLib is generic. It operates within the scope of HII database
> definitions from UEFI specification.
> > The new HiiGetUserSelection you are proposing deals with presentation, which means
> it is may get changed to match project UI.
> > HiiGetUserSelection can be added to the same new UiLib that I'm proposing.
> > [[Eric]] My proposal bases on modal form defined in UEFI Spec. Modal form UI
> decides
> > by the browser. Base on this reason, so I put this new API to the HiiLib. I only
> add one
> > new API and related to HII, so I prefer not add new library class.
> >
> > I think you want to split the API to new library just want to reduce the override
> code size? Or other reason?
> > [[[Felix]]] 1. I agree, if new function is based on standard HII Modal form, HiiLib
> is a good place for it.
> > 2. Yes, the main reason is a clean project porting.
> > Today I have two options:
> > Option 1: replace CreatePopUp implementation in Console.c with my custom
> implementation
> > Option 2: create custom instance of UefiLib
> > With both options, I'm deviating from standard EDKII code base, which means I will
> have to merge my changes whenever I want to
> > upgrade EDKII tree.
> > Also I will have to manually migrate the patch from project to project.
> >
> >
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dong, Eric
> > Sent: Monday, October 31, 2016 4:26 AM
> > To: Felix Poludov; edk2-devel@lists.01.org
> > Cc: Bi, Dandan; Gao, Liming
> > Subject: Re: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> >
> > Hi Felix,
> >
> > Add my comments below.
> >
> > > -----Original Message-----
> > > From: Felix Poludov [mailto:Felixp@ami.com]
> > > Sent: Friday, October 28, 2016 9:52 PM
> > > To: Dong, Eric; edk2-devel@lists.01.org
> > > Cc: Gao, Liming; Bi, Dandan
> > > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> > >
> > > Hi Eric,
> > >
> > > My goal is to facilitate CreatePopUp customization.
> > > Since UI is one of the most customizable areas in the firmware projects, an
> ability to easily replace UI element would be useful.
> > > Thank you for providing the presentation.
> > > I agree with the problem statement. It describes some of the reasons behind my
> request.
> > > As far as the solution you propose, you are introducing a new function
> > > HiiGetUserSelection, which is more powerful, but still implements a specific
> look-and-feel.
> > > So it should be possible to easily replace HiiGetUserSelection with a
> > > project specific version to align implementation with project-specific UI.
> > > Which library class are you planning to add HiiGetUserSelection to?
> > [[Eric]] I plan to add this API to HiiLib which existed at
> MdeModulePkg/Library/UefiHiiLib. I think it's belong to HII scope.
> >
> > > Another question is, what are you planning to do with the existing CreatePopUp
> function?
> > [[Eric]] I plan to not change it. Just suggest user to use new API and deprecated
> it later.
> >
> > > If you just remove it, existing projects that use the function will break.
> > > With my proposal, CreatePopUp can be easily replaced by picking a different
> library instance.
> > > For example, we can have a legacy instance that implements current
> > > behavior as well as and advanced instance that implements popup using HII
> infrastructure.
> > >
> > > Thanks
> > > Felix
> > >
> > > -----Original Message-----
> > > From: Dong, Eric [mailto:eric.dong@intel.com]
> > > Sent: Thursday, October 27, 2016 10:36 PM
> > > To: Felix Poludov; edk2-devel@lists.01.org
> > > Cc: Gao, Liming; Bi, Dandan
> > > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp
> > >
> > > Hi Felix,
> > >
> > > Do you want to provide a new solution for CreatePopup or just want to
> > > split CreatePopup from UefiLib?  We already has a proposal to provide new API to
> replace CreatePopup. This new API will use modal
> > form to paint the UI. Detail you can see the proposal in below link:
> > > https://github.com/ydong10/doc/blob/master/Use%20Modal%20form%20for%20
> > > CreatePopup%20API.pptx
> > >
> > > Thanks,
> > > Eric
> > > > -----Original Message-----
> > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > > Of Felix Poludov
> > > > Sent: Friday, October 28, 2016 5:12 AM
> > > > To: edk2-devel@lists.01.org
> > > > Subject: [edk2] [RFC] [MdePkg] UefiLib: CreatePopUp
> > > >
> > > > UefiLib library class (MdePkg ) includes CreatePopUp function.
> > > > The function displays a message box.
> > > > There is certainly more than one way to draw a message box.
> > > > If homogenous user interface is a project requirement, CreatePopUp
> > > > is likely to be overridden to align message box appearance with the platform
> look and feel.
> > > > The function can be overridden by creating a project specific
> > > > UefiLib instance, but this seems like an overkill because the rest of the
> UefiLib, which is quite big, would have to be duplicated.
> > > >
> > > > One way to solve the problem is to move CreatePopUp to a new library class,
> however, this may break existing projects.
> > > > I suggest changing CreatePopUp implementation to delegate pop up
> > > > drawing to a new function UiCreatePopUp provided by a new library class
> UiLib.h.
> > > >
> > > > I would like to solicit feedback for this proposal.
> > > > If there will be no major objections, I'll start working on a patch.
> > > >
> > > > Thanks
> > > > Felix
> > > >
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and proprietary to
> American Megatrends, Inc.  This communication is intended to be read only by the
> individual or entity to whom it is addressed or by their designee. If the reader of
> this message is not the intended recipient, you are on notice that any distribution
> of this message, in any form, is strictly prohibited.  Please promptly notify the
> sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy
> all copies of the transmission.