Dear Shilpa, I vote for first, and get it from an env variable. Really no reason why to involve Elementary in the process. By not involving elementary you gain two major benefits. 1. Less complication because there are less things to change (especially because the change in ELM will have to be ugly and annoying). 2. There's no reason why Edje only applications shouldn't benefit from this change.
-- Tom. On Fri, 2011-04-29 at 03:49 +0000, SHILPAONKAR SINGH wrote: > Hi Tom, > > > > Further patches will depend on the decision we take regarding timer > > once decided I will resend patches by splitting them. > > > > Dear All, > > Please let us know your opinion. > > 1. Create timer internally in edje, use a variable in theme to decide > timer value, use a config variable of EDJE > > to decide the password mode[show last input/normal]. - simple and > clear way. > > (or) > > 2. Use a config variable of elementary to decide password mode[show > last/normal] , use a new theme for password-showlast > > mode. control timer[in and a new action] using a new theme. - A little > complicated way which would avoid timer in edje. > > > > From My side I have already done second design, I will start working > on first design as well. > > > > Thanks & Regards > > Shilpa Singh > > > > ------- Original Message ------- > > Sender : Tom Hacohen<tom.haco...@partner.samsung.com> > Engineer/STRI-SLP RTL Language supporting/Samsung Electronics > > Date : Apr 28, 2011 23:28 (GMT+09:00) > > Title : Re: Re: Re: [E-devel] [Patch] Entry - Password show last input > patch > > > > Dear Shilpa, > > Please start by splitting the patches and sending two different > patches. > It's really hard for me to further review the patches like this (that > I > don't know what I need to review and what not). > > Regarding what we discussed. Lets wait for others to reply, though > honestly, I like setting time in theme and actually doing it in edje > instead of setting a timer in the theme and implementing the timer > there. > > -- > Tom. > > On Thu, 2011-04-28 at 12:03 +0000, SHILPAONKAR SINGH wrote: > > Dear Tom and Raster, > > > > Please find my reply inline in a more visible manner :). > > > > > > > Please find my reply inline. > > > 1. Looking at the changes you did for elementary: I don't see why > > you > > > even need them. Just change the env var to > > EDJE_ENTRY_PASSWORD_SHOW_LAST > > > and skip all the things you had to do in elm... Makes no sense to > > have > > > those in ELM. By doing that you get the added value of supporting > > this > > > feature in edje (without elm) which is wanted anyway. > > > 2. Bullet #1 eliminates the need for > > > "entry_mode: PASSWORD_SHOW_LAST_INPUT;" You just need to set it as > > > password like it was, and merge the theme with the one of the > > regular > > > password. This will simplify some of your edje changes. > > > Elementary: Thanks for your comments, we also had this > > discussion long back :) > > > 1,2. Q) why do we have 2 themes?: Ans: entry_mode: > > PASSWORD_SHOW_LAST_INPUT is not the only difference, > > > the major difference is in program section of these 2 themes. In > the > > showlastinput mode, two new programs have been > > > added which are used to control the timer part. As you know the > > password is visible for some time and is masked after some 2 > seconds. > > > a new action HIDE_VISIBLE_PASSWORD is created just for this > purpose. > > we use "in" to control the timer and when timer expires this action > > > is triggered which calls a callback to remove the password tags. > > [ This was Raster's Idea and when we had discussion, you also had > > agreed with > > > this idea :) ] By having a theme we can contol the timer value via > > theme and eliminate the timer functionality from edje totally. > Please > > refer line numbers > > > 143-154 in the elm_entry.patch. hence we use config variable in > > elementary. > > > > Well, I'm not against controlling that in theme, I'm just wondering, > > why > > not support SHOW_LAST in edje as well? all the logic is done in edje > > anyway (except for the timer), so why not remove the timer to inside > > of > > edje and just make that simpler without the need for another > program? > > (Like I suggested above). Maybe I've agreed with that in the past (I > > honestly don't remember), but seeing it now it really looks odd and > > complex for no reason.... We already have too many duplicated groups > > in > > ELM as it is, no need to dupe it more. > > > > We can just add a property in the theme that tells edje the delay > time > > (with edje having a default value of its own) which means it'll be > > configurable by elementary and will just work for any edje app. This > > will remove all this unneeded complexity and will make everything a > > lot > > clearer. I think this is the way to go, what do you think? > > > > (And create a timer in C inside the edje code instead of creating a > > hackish timer in the theme...) > > > > The way suggested by you was the same way I was following before > > My Old code created a ecore timer internally. Definitely this is a > > more simple and understandable way. > > but I had to change it as Raster wanted me to remove timer from edje > > and do it via theme. > > using HIDE_VISIBLE_PASSWORD action and "in". I think lets confirm > > this > > approach with Raster again before I send a new patch. please check > the > > chat below which we > > had last time regarding this issue. > > > > > > > > Hi Raster, > > > > Kindly let me know your opinion. > > > > [ > > > > in custom password two new programs have been added, these > > programs > > > > are used for the timer > > > > > > > > part[ using in and a new action HIDE_VISIBLE_PASSWORD is > created > > > > just for this purpose]. so As two themes > > > > > > > > are used two edit modes are used at edje level[ PASSWORD and > > > > PASSWORD_SHOW_LAST_CHARACTER]. > > > > > > > > I did confirm the same with Raster before starting, > > that > > > > I have 2 themes for password mode at entry level and he > > > > > > > > said thats fine. > > > > > > > > > > > > > > > > Raster what do you think about Tom's Idea? kindly let me know > > your > > > > opinion. > > > Ok, saw raster's reply, so never mind this comment. > > ] > > > > > > > 3. I noticed you did a bunch of changes in edje (near preedit > > mostly), > > > what are they for? > > > > > > For Multitap Input 3x4 keyboard type and languages type of > > input where we need to tap multiple > > > times to arrive at a character - This fix is only for > sofetkeyboard > > i;e, ise. > > > so you can check commit_cb and preedit_changed_cb, I add tag on > > pre-edit and remove the tag on > > > commit but if there is no pre-edit string then I directly add the > > tag on commit. > > > so until the user arrives at the required character in 3x4 or > > language input user will be able to view the character > > > once the character is committed it automatically turns in to *. > > > The features have been tested with 3x4 input and language input on > > the device. > > > > Well, those changes aren't really related to password mode, they are > > just to fix bugs in preedit handling so please split them to another > > patch. It's confusing to follow/review. > > > > I will do one thing I will only send the patch for keyboard and for > > softkey board I will > > send one more patch. > > > > > > > 4. Regarding textblock: I understand why you did your change to > > > evas_textblock_node_format_remove_pair, but the change isn't the > > correct > > > way to handle this. I'm currently working on fixing this function, > > my > > > bad for forgetting about it. :P > > > When I called the function, though the tags were removed > the > > text > > > was not immediately rendered hence I called the invalidate > function > > to > > > render the text immediately. My requirement is that when I remove > > the tag > > > the display of text has to adjust immediately else the password > > feature would be incomplete. > > > > Yes, I fixed that in upstream, would you mind checking? > > > > It works fine I updated and tested and Thanks for merging the patch. > > > > Thanks & Regards > > Shilpa Singh > > > > > > > > ------- Original Message ------- > > Sender : Tom Hacohen > > Engineer/STRI-SLP RTL Language supporting/Samsung Electronics > > Date : Apr 28, 2011 18:42 (GMT+09:00) > > Title : Re: Re: [E-devel] [Patch] Entry - Password show last input > > patch > > > > Dear Shilpa, > > > > Please mark your changes in a more visible way next time. :P I can't > > understand who's who. > > > > My comments are also inlined. > > On Thu, 2011-04-28 at 09:21 +0000, SHILPAONKAR SINGH wrote: > > > Dear Tom, > > > > > > Please find my reply inline. > > > 1. Looking at the changes you did for elementary: I don't see why > > you > > > even need them. Just change the env var to > > EDJE_ENTRY_PASSWORD_SHOW_LAST > > > and skip all the things you had to do in elm... Makes no sense to > > have > > > those in ELM. By doing that you get the added value of supporting > > this > > > feature in edje (without elm) which is wanted anyway. > > > 2. Bullet #1 eliminates the need for > > > "entry_mode: PASSWORD_SHOW_LAST_INPUT;" You just need to set it as > > > password like it was, and merge the theme with the one of the > > regular > > > password. This will simplify some of your edje changes. > > > Elementary: Thanks for your comments, we also had this > > discussion long back :) > > > 1,2. Q) why do we have 2 themes?: Ans: entry_mode: > > PASSWORD_SHOW_LAST_INPUT is not the only difference, > > > the major difference is in program section of these 2 themes. In > the > > showlastinput mode, two new programs have been > > > added which are used to control the timer part. As you know the > > password is visible for some time and is masked after some 2 > seconds. > > > a new action HIDE_VISIBLE_PASSWORD is created just for this > purpose. > > we use "in" to control the timer and when timer expires this action > > > is triggered which calls a callback to remove the password tags. > > [ This was Raster's Idea and when we had discussion, you also had > > agreed with > > > this idea :) ] By having a theme we can contol the timer value via > > theme and eliminate the timer functionality from edje totally. > Please > > refer line numbers > > > 143-154 in the elm_entry.patch. hence we use config variable in > > elementary. > > > > Well, I'm not against controlling that in theme, I'm just wondering, > > why > > not support SHOW_LAST in edje as well? all the logic is done in edje > > anyway (except for the timer), so why not remove the timer to inside > > of > > edje and just make that simpler without the need for another > program? > > (Like I suggested above). Maybe I've agreed with that in the past (I > > honestly don't remember), but seeing it now it really looks odd and > > complex for no reason.... We already have too many duplicated groups > > in > > ELM as it is, no need to dupe it more. > > > > We can just add a property in the theme that tells edje the delay > time > > (with edje having a default value of its own) which means it'll be > > configurable by elementary and will just work for any edje app. This > > will remove all this unneeded complexity and will make everything a > > lot > > clearer. I think this is the way to go, what do you think? > > > > (And create a timer in C inside the edje code instead of creating a > > hackish timer in the theme...) > > > > -- > > Tom. > > > > > > 3. I noticed you did a bunch of changes in edje (near preedit > > mostly), > > > what are they for? > > > > > > For Multitap Input 3x4 keyboard type and languages type of > > input where we need to tap multiple > > > times to arrive at a character - This fix is only for > sofetkeyboard > > i;e, ise. > > > so you can check commit_cb and preedit_changed_cb, I add tag on > > pre-edit and remove the tag on > > > commit but if there is no pre-edit string then I directly add the > > tag on commit. > > > so until the user arrives at the required character in 3x4 or > > language input user will be able to view the character > > > once the character is committed it automatically turns in to *. > > > The features have been tested with 3x4 input and language input on > > the device. > > > > Well, those changes aren't really related to password mode, they are > > just to fix bugs in preedit handling so please split them to another > > patch. It's confusing to follow/review. > > > > > > 4. Regarding textblock: I understand why you did your change to > > > evas_textblock_node_format_remove_pair, but the change isn't the > > correct > > > way to handle this. I'm currently working on fixing this function, > > my > > > bad for forgetting about it. :P > > > When I called the function, though the tags were removed > the > > text > > > was not immediately rendered hence I called the invalidate > function > > to > > > render the text immediately. My requirement is that when I remove > > the tag > > > the display of text has to adjust immediately else the password > > feature would be incomplete. > > > > Yes, I fixed that in upstream, would you mind checking? > > > > > > Thanks & Regards > > > Shilpa Singh > > > > > > ------- Original Message ------- > > > Sender : Tom Hacohen Engineer/STRI-SLP RTL Language > > supporting/Samsung Electronics > > > Date : Apr 28, 2011 16:58 (GMT+09:00) > > > Title : Re: [E-devel] [Patch] Entry - Password show last input > patch > > > > > > Dear Shilpa, > > > > > > Here are my comments: > > > 1. Looking at the changes you did for elementary: I don't see why > > you > > > even need them. Just change the env var to > > EDJE_ENTRY_PASSWORD_SHOW_LAST > > > and skip all the things you had to do in elm... Makes no sense to > > have > > > those in ELM. By doing that you get the added value of supporting > > this > > > feature in edje (without elm) which is wanted anyway. > > > 2. Bullet #1 eliminates the need for > > > "entry_mode: PASSWORD_SHOW_LAST_INPUT;" You just need to set it as > > > password like it was, and merge the theme with the one of the > > regular > > > password. This will simplify some of your edje changes. > > > 3. I noticed you did a bunch of changes in edje (near preedit > > mostly), > > > what are they for? > > > 4. Regarding textblock: I understand why you did your change to > > > evas_textblock_node_format_remove_pair, but the change isn't the > > correct > > > way to handle this. I'm currently working on fixing this function, > > my > > > bad for forgetting about it. :P > > > > > > Please fix/explain the above comments and then I will review the > > patch > > > again. > > > > > > Thanks, > > > Tom. > > > > > > > > > On Thu, 2011-04-28 at 14:38 +0900, Daniel Juyung Seo wrote: > > > > Dear all, I forward Shilpaonkar Singh's entry patch to mailing > > list. > > > > Please find the attached mail and patch files. > > > > > > > > > > > > ------- Original Message ------- > > > > > > > > Sender : SHILPAONKAR SINGH > > > > Date : Apr 27, 2011 22:22 (GMT+09:00) > > > > Title : Entry - [Password show last input] open source patch > > > > > > > > Hi Mr. Seo, > > > > > > > > Please find attached patches for last input show password > feature. > > > > These patches are based on current SVN version - 58957. > > > > > > > > There is no special demo app for this feature, for testing > purpose > > > > enviromental variable > > > > ELM_PASSWORD_SHOW_LAST_INPUT has to be set as shown below. > > > > export ELM_PASSWORD_SHOW_LAST_INPUT = 1 and then by clicking on > > > > entry_scrolled > > > > we can test password feature. > > > > > > > > I have also done a fix in evas_textblock_node_format_remove_pair > > function > > > > for this feature in the current svn version. > > > > Can you please push this patch to community as there are alot of > > changes > > > > happening in textblock level it would be better > > > > to push it soon to avoid rework. > > > > > > > > Thanks & Regards > > > > > > > > Shilpa Singh > > > > > > > > > > > ------------------------------------------------------------------------------ > > > > WhatsUp Gold - Download Free Network Management Software > > > > The most intuitive, comprehensive, and cost-effective network > > > > management toolset available today. Delivers lowest initial > > > > acquisition cost and overall TCO of any competing solution. > > > > http://p.sf.net/sfu/whatsupgold-sd > > > > _______________________________________________ > > > > enlightenment-devel mailing list > > > > enlightenment-devel@lists.sourceforge.net > > > > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > > > > > > > > > > > > > > > > > > Thanks & Regards > > > shilpa singh > > > > > > > > > > > > Thanks & Regards > > shilpa singh > > > > > > > > > > > Thanks & Regards > > shilpa singh > > > > > > > > ------------------------------------------------------------------------------ WhatsUp Gold - Download Free Network Management Software The most intuitive, comprehensive, and cost-effective network management toolset available today. Delivers lowest initial acquisition cost and overall TCO of any competing solution. http://p.sf.net/sfu/whatsupgold-sd _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
