From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 EEE2781D36 for ; Thu, 3 Nov 2016 00:32:44 -0700 (PDT) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP; 03 Nov 2016 00:32:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,585,1473145200"; d="scan'208";a="27232385" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga004.jf.intel.com with ESMTP; 03 Nov 2016 00:32:45 -0700 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 3 Nov 2016 00:32:44 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 3 Nov 2016 00:32:44 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.206]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.96]) with mapi id 14.03.0248.002; Thu, 3 Nov 2016 15:32:40 +0800 From: "Dong, Eric" To: Felix Poludov , "edk2-devel@lists.01.org" CC: "Bi, Dandan" , "Gao, Liming" Thread-Topic: [RFC] [MdePkg] UefiLib: CreatePopUp Thread-Index: AdIwjdjhOCSq+FCtTKCTGmtOXAGI2QAMMLIgABg3kNAAjBbicAAXwstQABbm8XAAVGQNQAARaGqw Date: Thu, 3 Nov 2016 07:32:39 +0000 Message-ID: References: <9333E191E0D52B4999CE63A99BA663A00270FF754E@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF787F@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF7D5B@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00270FF860F@atlms1.us.megatrends.com> In-Reply-To: <9333E191E0D52B4999CE63A99BA663A00270FF860F@atlms1.us.megatrends.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] 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: Thu, 03 Nov 2016 07:32:45 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 fi= le need to include the new UiLib instance.=20 I think the new UiLib library class need to be added into MdePkg because U= efiLib need to depend on it. This may need get some input from MdePkg maint= ainer. If you add this new UiLib, I will add my new API into this library c= lass. Thanks, Eric > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fe= lix 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 >=20 > Eric, >=20 > See my comments below >=20 > -----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 >=20 > Felix, >=20 > Add my comments below. >=20 > -----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 >=20 > Eric, >=20 > 1. If you are not changing CretePopUp, your proposal does not really solv= e my problem. > It means my proposal still has merit. > Do you have problem with moving CreatePopUp implementation into a new lib= rary class? > Once again, the function will stay in UefiLib, but the implementation wil= l 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 m= odal 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. >=20 > [[[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/Ue= fiLib/UefiLib.inf. New library class UiLib is added there. > 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 class= is added. The library contains UiCreatePopUp function. > Function implementation is copied from original CreatePopUp implementatio= n in MdePkg/Library/UefiLib/Console.c. >=20 > To summarize: > - There is no need to change code that consumes CreatePopUp because the f= unction is still there > - CreatePopUp consumers see no difference in look-and-feel and behavior b= ecause 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... >=20 > 2. In my opinion, adding HiiGetUserSelection to HiiLib is a bad idea beca= use it will create the same problem as with CretePopUp. > The rest of HiiLib is generic. It operates within the scope of HII databa= se definitions from UEFI specification. > The new HiiGetUserSelection you are proposing deals with presentation, wh= ich 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. >=20 > 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 fo= rm, 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 mea= ns 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. >=20 >=20 > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Do= ng, 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 >=20 > Hi Felix, >=20 > Add my comments below. >=20 > > -----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 be= hind my request. > > As far as the solution you propose, you are introducing a new function > > HiiGetUserSelection, which is more powerful, but still implements a spe= cific 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/L= ibrary/UefiHiiLib. I think it's belong to HII scope. >=20 > > Another question is, what are you planning to do with the existing Crea= tePopUp function? > [[Eric]] I plan to not change it. Just suggest user to use new API and de= precated it later. >=20 > > If you just remove it, existing projects that use the function will bre= ak. > > With my proposal, CreatePopUp can be easily replaced by picking a diffe= rent library instance. > > For example, we can have a legacy instance that implements current > > behavior as well as and advanced instance that implements popup using H= II 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 n= ew 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 p= latform 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 cla= ss 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 notif= y 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 th= e 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 >=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 addres= sed or by their designee. If the reader of this message is not the > intended recipient, you are on notice that any distribution of this messa= ge, 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 o= r destroy all copies of the transmission. >=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 addres= sed or by their designee. If the reader of this message is not the > intended recipient, you are on notice that any distribution of this messa= ge, 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 o= r destroy all copies of the transmission. > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel