From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3014C81D65 for ; Thu, 3 Nov 2016 19:39:02 -0700 (PDT) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 03 Nov 2016 19:39:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,589,1473145200"; d="scan'208";a="1063720624" Received: from orsmsx108.amr.corp.intel.com ([10.22.240.6]) by fmsmga001.fm.intel.com with ESMTP; 03 Nov 2016 19:39:04 -0700 Received: from orsmsx160.amr.corp.intel.com (10.22.226.43) by ORSMSX108.amr.corp.intel.com (10.22.240.6) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 3 Nov 2016 19:39:03 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.250]) by ORSMSX160.amr.corp.intel.com ([10.22.226.43]) with mapi id 14.03.0248.002; Thu, 3 Nov 2016 19:39:03 -0700 From: "Kinney, Michael D" To: Felix Poludov , "Dong, Eric" , "edk2-devel@lists.01.org" , "Kinney, Michael D" CC: "Bi, Dandan" , "Gao, Liming" Thread-Topic: [RFC] [MdePkg] UefiLib: CreatePopUp Thread-Index: AdIwjdjhOCSq+FCtTKCTGmtOXAGI2QAMMLIgABg3kNAAjBbicAAXwstQABbm8XAAVGQNQAARaGqwAB3IF9AABgDy8A== Date: Fri, 4 Nov 2016 02:39:02 +0000 Message-ID: References: <9333E191E0D52B4999CE63A99BA663A00270FF754E@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF787F@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF7D5B@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF860F@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF8B52@atlms1.us.megatrends.com> In-Reply-To: <9333E191E0D52B4999CE63A99BA663A00270FF8B52@atlms1.us.megatrends.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGQ3NjhkNmQtM2UxZi00OTJiLWIzYTctNmNhNWI3ODU5YmYxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlMwOVA2U01Ba2dlbFcwYTkrRkhyRFVnU1BudzRrcHhcL1NtRjViaVZyeUpVPSJ9 x-originating-ip: [10.22.254.140] MIME-Version: 1.0 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: Fri, 04 Nov 2016 02:39:02 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Felix, >>From the thread, your description is missing the update to every platform D= SC file to add the new UiLib class to instance mapping even if the platform does no= t want=20 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 requi= res a DSC file update, doing the lib layering you describe is not required. You = could=20 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=20 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 ; edk2-devel@lists.01.org; Kinney, Mi= chael D > > Cc: Bi, Dandan ; Gao, Liming > Subject: RE: [RFC] [MdePkg] UefiLib: CreatePopUp >=20 > Mike, >=20 > 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 u= pgrades and > feature migration from project to project becomes more complicated) >=20 > I propose changing CreatePopUp implementation to delegate pop up > drawing to a new function UiCreatePopUp provided by a new library class U= iLib.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 ther= e. > 3. CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is rep= laced 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 clas= s is added. > The library contains UiCreatePopUp function. > Function implementation is copied from original CreatePopUp implementati= on in > MdePkg/Library/UefiLib/Console.c. >=20 > Thanks > Felix >=20 > -----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 >=20 > Hi Felix, >=20 > Thanks for your clearly explanation, I misunderstand your changes before.= But I > think this change still has small impact because the platforms DSC file n= eed to > include the new UiLib instance. >=20 > 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. >=20 > 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 ; 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 so= lve my > problem. > > It means my proposal still has merit. > > Do you have problem with moving CreatePopUp implementation into a new l= ibrary > class? > > Once again, the function will stay in UefiLib, but the implementation w= ill 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 w= e must avoid > it. > > It will impact a lot of core codes which use CreatePopup API. > > > > [[[Felix]]] Since you have no plans to change CreatePopUp implementatio= n, 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 backwa= rd > 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 ther= e. > > 3. CreatePopUp implementation in MdePkg/Library/UefiLib/Console.c is r= eplaced > 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 cla= ss is added. > The library contains UiCreatePopUp function. > > Function implementation is copied from original CreatePopUp implementat= ion 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 clas= s. > > I agree that creating a new library class for a single function does no= t 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 be= cause it > will create the same problem as with CretePopUp. > > The rest of HiiLib is generic. It operates within the scope of HII data= base > 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 proposi= ng. > > [[Eric]] My proposal bases on modal form defined in UEFI Spec. Modal fo= rm UI > decides > > by the browser. Base on this reason, so I put this new API to the HiiLi= b. 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 th= e 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 custo= m > implementation > > Option 2: create custom instance of UefiLib > > With both options, I'm deviating from standard EDKII code base, which m= eans 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 projec= ts, 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 functio= n > > > HiiGetUserSelection, which is more powerful, but still implements a s= pecific > look-and-feel. > > > So it should be possible to easily replace HiiGetUserSelection with a > > > project specific version to align implementation with project-specifi= c 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 Cr= eatePopUp > 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 b= reak. > > > With my proposal, CreatePopUp can be easily replaced by picking a dif= ferent > 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%2= 0 > > > 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 librar= y 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 c= lass > 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 > > > > >=20 > Please consider the environment before printing this email. >=20 > The information contained in this message may be confidential and proprie= tary 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 di= stribution > of this message, in any form, is strictly prohibited. Please promptly no= tify the > sender by reply e-mail or by telephone at 770-246-8600, and then delete o= r destroy > all copies of the transmission.