Quantcast

[PINENTRY PATCH] add efl-based frontend

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PINENTRY PATCH] add efl-based frontend

Mike Blumenkrantz-2
Hi,

Attached is a patch implementing an efl-based gui frontend for pinentry.

As a note, the formatting in pinentry-efl.c was done by using the 'indent' tool; I am not familiar enough with this coding style to manually make corrections if further changes are needed in this regard.

Thanks for your time and review,
Mike

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

0001-add-efl-based-frontend-for-pinentry.patch (34K) Download Attachment
attachment1 (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Justus Winter
Hi Mike :)

Mike Blumenkrantz <[hidden email]> writes:
> Attached is a patch implementing an efl-based gui frontend for pinentry.

Cool!

> As a note, the formatting in pinentry-efl.c was done by using the
> 'indent' tool; I am not familiar enough with this coding style to
> manually make corrections if further changes are needed in this
> regard.

The formatting of the code looks nice, thanks.

The formatting of the commit message, however, does not.  Details can
be found in the already mentioned doc/HACKING file.  At the very least,
try to make it blend in with the other commit messages ;)

> Thanks for your time and review,

I tried to compile it, but failed to do so because it depends on a very
recent libelementary which is only available from Debian experimental,
and I failed to pull that in for some reason.

In any case, that means that your pinentry won't be usable for the
majority until the required version reaches end users.  You might want
to try to relax this a little (though ofc I don't know anything about
libelementary, so...).

But I noticed that despite me using ../configure --enable-pinentry-efl,
the failed version check was not fatal.  I'd say it should abort if the
user explicitly asked for it but the requirements were not met.


Cheers,
Justus

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

signature.asc (463 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Mike Blumenkrantz
Hey, thanks for the quick feedback!

On Thu, Oct 13, 2016 at 8:28 AM Justus Winter <[hidden email]> wrote:
Hi Mike :)



Mike Blumenkrantz <[hidden email]> writes:

> Attached is a patch implementing an efl-based gui frontend for pinentry.



Cool!



> As a note, the formatting in pinentry-efl.c was done by using the

> 'indent' tool; I am not familiar enough with this coding style to

> manually make corrections if further changes are needed in this

> regard.



The formatting of the code looks nice, thanks.



The formatting of the commit message, however, does not.  Details can

be found in the already mentioned doc/HACKING file.  At the very least,

try to make it blend in with the other commit messages ;)

Can you elaborate on exactly which parts need changes? I tried to follow the policy in that file, but there did not seem to be much which could apply to adding new frontends like this.
 



> Thanks for your time and review,



I tried to compile it, but failed to do so because it depends on a very

recent libelementary which is only available from Debian experimental,

and I failed to pull that in for some reason.



In any case, that means that your pinentry won't be usable for the

majority until the required version reaches end users.  You might want

to try to relax this a little (though ofc I don't know anything about

libelementary, so...).

Regrettably, reducing the version requirements is not possible in order for the frontend to have readable text. I'm fine with it not being usable for some number of users at this time.
 



But I noticed that despite me using ../configure --enable-pinentry-efl,

the failed version check was not fatal.  I'd say it should abort if the

user explicitly asked for it but the requirements were not met.

Ah, I'll fix this, thanks.
 





Cheers,

Justus

_______________________________________________

Gnupg-devel mailing list

[hidden email]

http://lists.gnupg.org/mailman/listinfo/gnupg-devel


_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Justus Winter
Mike Blumenkrantz <[hidden email]> writes:
> On Thu, Oct 13, 2016 at 8:28 AM Justus Winter <[hidden email]> wrote:
>> The formatting of the commit message, however, does not.  Details can
>> be found in the already mentioned doc/HACKING file.  At the very least,
>> try to make it blend in with the other commit messages ;)
>
> Can you elaborate on exactly which parts need changes? I tried to follow
> the policy in that file, but there did not seem to be much which could
> apply to adding new frontends like this.

We write whole sentences, the first word starts with a capital letter,
and we end sentences with a period.  We include ChangeLog-style entries
for all affected files.  We start with a one-line summary that starts
with a component followed by a colon, a space, and a whole sentence.

All of that should be obvious by just averaging over some of the recent
commit messages even without looking at the documentation.

I realize that all of that is a matter of taste, and one cannot expect
everyone to hunt down the docs, and follow it to the letter.  I don't
believe we are particularly picky about this, but your message really
sticks out.


Cheers,
Justus

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

signature.asc (463 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Mike Blumenkrantz-2
On Wed, 19 Oct 2016 14:07:41 +0200
Justus Winter <[hidden email]> wrote:

> Mike Blumenkrantz <[hidden email]> writes:
> > On Thu, Oct 13, 2016 at 8:28 AM Justus Winter <[hidden email]>
> > wrote:  
> >> The formatting of the commit message, however, does not.  Details
> >> can be found in the already mentioned doc/HACKING file.  At the
> >> very least, try to make it blend in with the other commit
> >> messages ;)  
> >
> > Can you elaborate on exactly which parts need changes? I tried to
> > follow the policy in that file, but there did not seem to be much
> > which could apply to adding new frontends like this.  
>
> We write whole sentences, the first word starts with a capital letter,
> and we end sentences with a period.  We include ChangeLog-style
> entries for all affected files.  We start with a one-line summary
> that starts with a component followed by a colon, a space, and a
> whole sentence.
>
> All of that should be obvious by just averaging over some of the
> recent commit messages even without looking at the documentation.
>
> I realize that all of that is a matter of taste, and one cannot expect
> everyone to hunt down the docs, and follow it to the letter.  I don't
> believe we are particularly picky about this, but your message really
> sticks out.
>
>
> Cheers,
> Justus
Hi,

Thanks for your clarification. I've attached a new version of the patch
which I believe addresses all the issues you have cited in both the
contents of the patch and the commit log.


Since we are being very technical about the commit log, and you
specifically cited the docs, I'd like to point out that nowhere in the
docs/HACKING file
(https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/HACKING#l21)
does it say anything about capitalization or punctuation. The prefixing
that you've mentioned, while common in commit messages, is referred to
something that is "usually" present, and it has no example for the
addition of new components. Furthermore, listing files with their
changes is also not in this document, nor is it done in every commit:
the top-most commit that I was working off in the tree is
https://git.gnupg.org/cgi-bin/gitweb.cgi?p=pinentry.git;a=commit;h=a383ddeb76463ddcf5aca2fb38847ea3158c42a7 and does not include it.
Based on this disconnect, it's easy to see why someone like
me who has been working off very direct RFC-style specifications
recently would produce a commit log which "stands out" :)

I'd be happy to submit a patch for the docs/HACKING file which
addresses these discrepancies to avoid any potential confusion
with future contributors.

Regards,
Mike

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

0001-efl-Add-an-efl-based-frontend-for-pinentry.patch (34K) Download Attachment
attachment1 (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Justus Winter
Mike Blumenkrantz <[hidden email]> writes:

> Thanks for your clarification. I've attached a new version of the patch
> which I believe addresses all the issues you have cited in both the
> contents of the patch and the commit log.

Thanks.  In the mean time, I was finally able to build the pinentry.  I
have some remarks:

1/ the window is very small, there is no margin around the widgets at
all, it looks crowded.

2/ as soon as I enter a character, the text input field is resized
(enlarged), and as a consequence so is the dialog window.

3/ the buttons have no logos, yet the text is off-center.

> Since we are being very technical about the commit log, and you
> specifically cited the docs, I'd like to point out that nowhere in the
> docs/HACKING file
> (https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/HACKING#l21)
> does it say anything about capitalization or punctuation. The prefixing
> that you've mentioned, while common in commit messages, is referred to
> something that is "usually" present, and it has no example for the
> addition of new components. Furthermore, listing files with their
> changes is also not in this document, nor is it done in every commit:
> the top-most commit that I was working off in the tree is
> https://git.gnupg.org/cgi-bin/gitweb.cgi?p=pinentry.git;a=commit;h=a383ddeb76463ddcf5aca2fb38847ea3158c42a7 and does not include it.
> Based on this disconnect, it's easy to see why someone like
> me who has been working off very direct RFC-style specifications
> recently would produce a commit log which "stands out" :)
>
> I'd be happy to submit a patch for the docs/HACKING file which
> addresses these discrepancies to avoid any potential confusion
> with future contributors.
Documentation patches are very welcome :)


Thanks,
Justus

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Mike Blumenkrantz-2
On Tue, 24 Jan 2017 12:10:52 +0100
Justus Winter <[hidden email]> wrote:

> Mike Blumenkrantz <[hidden email]> writes:
>
> > Thanks for your clarification. I've attached a new version of the patch
> > which I believe addresses all the issues you have cited in both the
> > contents of the patch and the commit log.  
>
> Thanks.  In the mean time, I was finally able to build the pinentry.  I
> have some remarks:
>
> 1/ the window is very small, there is no margin around the widgets at
> all, it looks crowded.
>
> 2/ as soon as I enter a character, the text input field is resized
> (enlarged), and as a consequence so is the dialog window.
>
> 3/ the buttons have no logos, yet the text is off-center.
Thanks for checking, I'll look into these items.

>
> > Since we are being very technical about the commit log, and you
> > specifically cited the docs, I'd like to point out that nowhere in the
> > docs/HACKING file
> > (https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/HACKING#l21)
> > does it say anything about capitalization or punctuation. The prefixing
> > that you've mentioned, while common in commit messages, is referred to
> > something that is "usually" present, and it has no example for the
> > addition of new components. Furthermore, listing files with their
> > changes is also not in this document, nor is it done in every commit:
> > the top-most commit that I was working off in the tree is
> > https://git.gnupg.org/cgi-bin/gitweb.cgi?p=pinentry.git;a=commit;h=a383ddeb76463ddcf5aca2fb38847ea3158c42a7 and does not include it.
> > Based on this disconnect, it's easy to see why someone like
> > me who has been working off very direct RFC-style specifications
> > recently would produce a commit log which "stands out" :)
> >
> > I'd be happy to submit a patch for the docs/HACKING file which
> > addresses these discrepancies to avoid any potential confusion
> > with future contributors.  
>
> Documentation patches are very welcome :)
>
>
> Thanks,
> Justus
I'll follow up on this as well.

Regards,
Mike

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

attachment0 (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Mike Blumenkrantz-2
On Wed, 25 Jan 2017 08:12:19 -0500
Mike Blumenkrantz <[hidden email]> wrote:

> On Tue, 24 Jan 2017 12:10:52 +0100
> Justus Winter <[hidden email]> wrote:
>
> > Mike Blumenkrantz <[hidden email]> writes:
> >  
> > > Thanks for your clarification. I've attached a new version of the patch
> > > which I believe addresses all the issues you have cited in both the
> > > contents of the patch and the commit log.    
> >
> > Thanks.  In the mean time, I was finally able to build the pinentry.  I
> > have some remarks:
> >
> > 1/ the window is very small, there is no margin around the widgets at
> > all, it looks crowded.
> >
> > 2/ as soon as I enter a character, the text input field is resized
> > (enlarged), and as a consequence so is the dialog window.
> >
> > 3/ the buttons have no logos, yet the text is off-center.  
>
> Thanks for checking, I'll look into these items.
>
> >  
> > > Since we are being very technical about the commit log, and you
> > > specifically cited the docs, I'd like to point out that nowhere in the
> > > docs/HACKING file
> > > (https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/HACKING#l21)
> > > does it say anything about capitalization or punctuation. The prefixing
> > > that you've mentioned, while common in commit messages, is referred to
> > > something that is "usually" present, and it has no example for the
> > > addition of new components. Furthermore, listing files with their
> > > changes is also not in this document, nor is it done in every commit:
> > > the top-most commit that I was working off in the tree is
> > > https://git.gnupg.org/cgi-bin/gitweb.cgi?p=pinentry.git;a=commit;h=a383ddeb76463ddcf5aca2fb38847ea3158c42a7 and does not include it.
> > > Based on this disconnect, it's easy to see why someone like
> > > me who has been working off very direct RFC-style specifications
> > > recently would produce a commit log which "stands out" :)
> > >
> > > I'd be happy to submit a patch for the docs/HACKING file which
> > > addresses these discrepancies to avoid any potential confusion
> > > with future contributors.    
> >
> > Documentation patches are very welcome :)
> >
> >
> > Thanks,
> > Justus  
>
> I'll follow up on this as well.
>
> Regards,
> Mike
Hi,

I've attached a revision of the patch which should address your comments:

1/ I've added some padding around the sides of the window

2/ Resizing is resolved, however there's currently a bug in EFL HEAD where initial window focus is broken; if you experience this while testing, it is not an application bug

3/ It sounds like you don't have an icon theme configured in efl, and no default is set. I've added some checks to prevent unusual alignments, but in the end it's a toolkit issue

Regards,
Mike

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

0001-efl-Add-an-efl-based-frontend-for-pinentry.patch (37K) Download Attachment
attachment1 (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Mike Blumenkrantz
In reply to this post by Mike Blumenkrantz-2
Hi,

I've sent a new version of the implementation to the list, but it's caught in moderation because the patch size is 41kb which exceeds the mailing list limit of 40kb.

Regards,
Mike

On Wed, Jan 25, 2017 at 9:42 AM Mike Blumenkrantz <[hidden email]> wrote:
On Tue, 24 Jan 2017 12:10:52 +0100
Justus Winter <[hidden email]> wrote:

> Mike Blumenkrantz <[hidden email]> writes:
>
> > Thanks for your clarification. I've attached a new version of the patch
> > which I believe addresses all the issues you have cited in both the
> > contents of the patch and the commit log.
>
> Thanks.  In the mean time, I was finally able to build the pinentry.  I
> have some remarks:
>
> 1/ the window is very small, there is no margin around the widgets at
> all, it looks crowded.
>
> 2/ as soon as I enter a character, the text input field is resized
> (enlarged), and as a consequence so is the dialog window.
>
> 3/ the buttons have no logos, yet the text is off-center.

Thanks for checking, I'll look into these items.

>
> > Since we are being very technical about the commit log, and you
> > specifically cited the docs, I'd like to point out that nowhere in the
> > docs/HACKING file
> > (https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/HACKING#l21)
> > does it say anything about capitalization or punctuation. The prefixing
> > that you've mentioned, while common in commit messages, is referred to
> > something that is "usually" present, and it has no example for the
> > addition of new components. Furthermore, listing files with their
> > changes is also not in this document, nor is it done in every commit:
> > the top-most commit that I was working off in the tree is
> > https://git.gnupg.org/cgi-bin/gitweb.cgi?p=pinentry.git;a=commit;h=a383ddeb76463ddcf5aca2fb38847ea3158c42a7 and does not include it.
> > Based on this disconnect, it's easy to see why someone like
> > me who has been working off very direct RFC-style specifications
> > recently would produce a commit log which "stands out" :)
> >
> > I'd be happy to submit a patch for the docs/HACKING file which
> > addresses these discrepancies to avoid any potential confusion
> > with future contributors.
>
> Documentation patches are very welcome :)
>
>
> Thanks,
> Justus

I'll follow up on this as well.

Regards,
Mike
_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Werner Koch
On Mon, 30 Jan 2017 15:59, [hidden email] said:

> in moderation because the patch size is 41kb which exceeds the mailing list
> limit of 40kb.

Approved and bumped the limit up to 60k.


Shalom-Salam,

   Werner

--
Die Gedanken sind frei.  Ausnahmen regelt ein Bundesgesetz.

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

attachment0 (233 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PINENTRY PATCH] add efl-based frontend

Mike Blumenkrantz-2
On Wed, 1 Feb 2017 16:47:10 +0100
Werner Koch <[hidden email]> wrote:

> On Mon, 30 Jan 2017 15:59, [hidden email] said:
>
> > in moderation because the patch size is 41kb which exceeds the mailing list
> > limit of 40kb.  
>
> Approved and bumped the limit up to 60k.
>
>
> Shalom-Salam,
>
>    Werner
>
Hi,

Is there any progress on reviewing this?

Regards,
Mike

_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

attachment0 (836 bytes) Download Attachment
Loading...