P.S: I committed the textblock part of the patch, that one was good, and I have no comments about that one.
-- Tom. On Thu, 2011-04-28 at 12:42 +0300, Tom Hacohen wrote: > 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<tom.haco...@partner.samsung.com> 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 > > > > ------------------------------------------------------------------------------ > 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 ------------------------------------------------------------------------------ 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
