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<tom.haco...@partner.samsung.com> > 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 > ------------------------------------------------------------------------------ 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
