From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from atlmailgw2.ami.com (atlmailgw2.ami.com [63.147.10.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 274F381C87 for ; Thu, 3 Nov 2016 16:55:09 -0700 (PDT) X-AuditID: ac10606f-da7ff70000000890-c0-581bce5ca4fd Received: from atlms2.us.megatrends.com (atlms2.us.megatrends.com [172.16.96.152]) (using TLS with cipher AES128-SHA (128/128 bits)) (Client did not present a certificate) by (Symantec Messaging Gateway) with SMTP id 6B.F9.02192.C5ECB185; Thu, 3 Nov 2016 19:55:10 -0400 (EDT) Received: from ATLMS1.us.megatrends.com ([fe80::8c55:daf0:ef05:5605]) by atlms2.us.megatrends.com ([fe80::29dc:a91e:ea0c:cdeb%12]) with mapi id 14.03.0123.003; Thu, 3 Nov 2016 19:55:08 -0400 From: Felix Poludov To: "Dong, Eric" , "edk2-devel@lists.01.org" , "Kinney, Michael D (michael.d.kinney@intel.com)" CC: "Bi, Dandan" , "Gao, Liming" Thread-Topic: [RFC] [MdePkg] UefiLib: CreatePopUp Thread-Index: AdIwjdjhOCSq+FCtTKCTGmtOXAGI2QAMMLIgABg3kNAAjBbicAAXwstQABbm8XAAVGQNQAARaGqwAB3IF9A= Date: Thu, 3 Nov 2016 23:55:07 +0000 Message-ID: <9333E191E0D52B4999CE63A99BA663A00270FF8B52@atlms1.us.megatrends.com> References: <9333E191E0D52B4999CE63A99BA663A00270FF754E@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF787F@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF7D5B@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF860F@atlms1.us.megatrends.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.16.99.93] content-transfer-encoding: quoted-printable MIME-Version: 1.0 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrCKsWRmVeSWpSXmKPExsWyRiBhhm7cOekIgy2beC22blO32HPoKLPF 5hfBFivubWC36Oj4x+TA6rF4z0smj+7Z/1gCmKIaGG0S8/LySxJLUhVSUouTbZUCijLLEpMr lRQyU2yVDJUUCnISk1NzU/NKbJUSCwpS81KU7LgUMIANUFlmnkJqXnJ+SmZeuq2SZ7C/roWF qaWuoZJdSEaqQmZeWn5RbmJJZn6eQnJ+XglQdWoKUFQhoYszY9rZ6cwFPyIq9k55yd7A2Oje xcjBISFgIvHqWn0XIxeHkMBsJonV766wdjFyAjmHGCX2v2ADsdkEVCQ2nb3ADFIkIrCOUWLN nA1gRcwCPhJL/m9gAhkkLGAgcfRKNEhYRMBQ4vGZm4wQdpLE801H2UFsFqA58yfOYQGxeQUC JTZs3ckMsesii8S5WYIgNqdAiMSTPSvA6hkFxCS+n1rDBLFKXOLWk/lgtoSAgMSSPeeZIWxR iZeP/7FC2AoSW953skPU60gs2P2JDcLWlli28DUzxF5BiZMzn7BMYBSdhWTsLCQts5C0zELS soCRZRWjUGJJTm5iZk56uZFeYm6mXnJ+7iZGSKLI38H48aP5IUYBDkYlHt7rm6QjhFgTy4or c4EhycGsJMI7+QhQiDclsbIqtSg/vqg0J7X4EKMTMFgmMktxg2ILGP3xxgYGUqIwjqGJmYm5 kbmhpYm5sbGSOK/0X99wIYF0YDLKTk0tSC2CGcLEwSnVwOi92yni/f2c/DUff23Zt17fb/Ji l737dcV1q5LWbufwm8pz8OOKjUpHY+dUZ9esvaT6/5la3L0VbfOOWa/vTAyuevh8R7Kr6TpX VsMjint0HoWatojPdtT/szp6hfrpKclPdj75xTN1p0Xc20eV6fzT3cor33e63woqELiVLJ5y ic3tqkb3SwMlluKMREMt5qLiRABfTkNSNwMAAA== Subject: Re: [RFC] [MdePkg] UefiLib: CreatePopUp X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Nov 2016 23:55:09 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" 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 imp= lementation Option 2: create custom instance of UefiLib Both options lead to project maintenance complications (EDKII code base upgr= ades 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 UiLi= b.h. More specifically: 1. No changes to UefiLib.h (CreatePopUp is still there) 2. A single line change in [LibraryClasses] section of MdePkg/Library/UefiL= ib/UefiLib.inf. New library class UiLib is added there. 3. CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is replac= ed 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 i= s 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. B= ut 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 Ue= fiLib need to depend on it. This may need get some input from MdePkg maintai= ner. If you add this new UiLib, I will add my new API into this library clas= s. Thanks, Eric > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fel= ix 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 ; edk2-devel@lists.01.org > Cc: Bi, Dandan ; Gao, Liming > 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 libr= ary 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 mo= dal 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 m= ust 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/Uef= iLib/UefiLib.inf. New library class UiLib is added there. > 3. CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is repl= aced 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 fu= nction is still there > - CreatePopUp consumers see no difference in look-and-feel and behavior be= cause 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 f= eel 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 becau= se it will create the same problem as with CretePopUp. > The rest of HiiLib is generic. It operates within the scope of HII databas= e definitions from UEFI specification. > The new HiiGetUserSelection you are proposing deals with presentation, whi= ch 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 o= verride code size? Or other reason? > [[[Felix]]] 1. I agree, if new function is based on standard HII Modal for= m, 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 i= mplementation > Option 2: create custom instance of UefiLib > With both options, I'm deviating from standard EDKII code base, which mean= s 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 Don= g, 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 beh= ind my request. > > As far as the solution you propose, you are introducing a new function > > HiiGetUserSelection, which is more powerful, but still implements a spec= ific look-and-feel. > > So it should be possible to easily replace HiiGetUserSelection with a > > project specific version to align implementation with project-specific U= I. > > Which library class are you planning to add HiiGetUserSelection to? > [[Eric]] I plan to add this API to HiiLib which existed at MdeModulePkg/Li= brary/UefiHiiLib. I think it's belong to HII scope. > > > Another question is, what are you planning to do with the existing Creat= ePopUp function? > [[Eric]] I plan to not change it. Just suggest user to use new API and dep= recated it later. > > > If you just remove it, existing projects that use the function will brea= k. > > With my proposal, CreatePopUp can be easily replaced by picking a differ= ent library instance. > > For example, we can have a legacy instance that implements current > > behavior as well as and advanced instance that implements popup using HI= I 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 ne= w 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 pl= atform 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 c= lass, 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 clas= s 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 proprietar= y to American Megatrends, Inc. This communication is intended to be read on= ly 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 not= ice that any distribution of this message, in any form, is strictly prohibit= ed. Please promptly notify the sender by reply e-mail or by telephone at 77= 0-246-8600, and then delete or destroy all copies of the transmission.