2008-11-24 18:51:21

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH] ncurses based config V2

Hi Nir.

Some comments as followup to Willy.

> OK, I've just tried it. Here are the first comments I can make :
>
> - colors are too dark.
Seconded. Please try to find a lighter set of default colors.
You may be inspired by one of the menuconfig color-sets.

I agree that we could use a lift here - so please do not go for the
default color set of menuconfig.

> - entering text in boxes (eg: local version) does not move the cursor,
> it remains at the beginning of the line. If I press any arrow, the
> box immediately closes (most likely the Esc prefix again).

I do not see any cursor which is annoying. menuconfig has the same issue.

> - I noticed I was tempted a lot to press "?" to get help, but the key is
> not bound. It would be nice to have it bound to Help since make oldconfig
> and menuconfig to both report help that way.

Please bound this.

> - I'm not convinced that the parenthesis around hotkeys make the menu
> that much readable, especially when there are lots of short words or
> even acronyms. Eg :
> [ ] (U)TS namespace
> [ ] (I)PC namespace
> [ ] (U)ser namespace (EXPERIMENTAL)
> [ ] (P)ID Namespaces (EXPERIMENTAL)

This looks like some ancient stuff and it must be a better way to
show the short-cuts.

I would really appreciate if you could look into the above issues.
If you do a repost with this fixed I will try to do a proper review,
including code-review next time.

Sam


2008-11-25 02:34:37

by Roman Zippel

[permalink] [raw]
Subject: Re: [PATCH] ncurses based config V2

Hi,

On Mon, 24 Nov 2008, Sam Ravnborg wrote:

> > - colors are too dark.
> Seconded. Please try to find a lighter set of default colors.
> You may be inspired by one of the menuconfig color-sets.
>
> I agree that we could use a lift here - so please do not go for the
> default color set of menuconfig.

Well, the current colors seems to usable on wide variety of terminals, but
I'm not colorblind, so I can't really complain. :)
Before we keep discussing the colors, I'd rather suggest to drop them
completely and maybe use colors for what actually needs attention.

> > - I'm not convinced that the parenthesis around hotkeys make the menu
> > that much readable, especially when there are lots of short words or
> > even acronyms. Eg :
> > [ ] (U)TS namespace
> > [ ] (I)PC namespace
> > [ ] (U)ser namespace (EXPERIMENTAL)
> > [ ] (P)ID Namespaces (EXPERIMENTAL)
>
> This looks like some ancient stuff and it must be a better way to
> show the short-cuts.

The parenthesis make it really annoying to read...

> I would really appreciate if you could look into the above issues.
> If you do a repost with this fixed I will try to do a proper review,
> including code-review next time.

The code is rather large, so it might be better to split the code into two
sources, one for kconfig logic and the other for the interface logic, so
it becomes easier to read and maintain.

bye, Roman

2008-11-25 08:01:31

by Nir Tzachar

[permalink] [raw]
Subject: Re: [PATCH] ncurses based config V2

Hello Sam.

On Mon, Nov 24, 2008 at 8:44 PM, Sam Ravnborg <[email protected]> wrote:
> Hi Nir.
>
> Some comments as followup to Willy.
>
>> OK, I've just tried it. Here are the first comments I can make :
>>
>> - colors are too dark.
> Seconded. Please try to find a lighter set of default colors.
> You may be inspired by one of the menuconfig color-sets.
>
> I agree that we could use a lift here - so please do not go for the
> default color set of menuconfig.

Personally, I like lighter text on a dark background. But this is just
me. I'll try to implement an easier color scheme chooser so everyone
will be able to play with it and come up with something they like.

>> - entering text in boxes (eg: local version) does not move the cursor,
>> it remains at the beginning of the line. If I press any arrow, the
>> box immediately closes (most likely the Esc prefix again).
>

This is fixed in my current version (I have not subitted a patch yet)

> I do not see any cursor which is annoying. menuconfig has the same issue.

also fixed.

>> - I noticed I was tempted a lot to press "?" to get help, but the key is
>> not bound. It would be nice to have it bound to Help since make oldconfig
>> and menuconfig to both report help that way.
>
> Please bound this.

consider it bounded..

>> - I'm not convinced that the parenthesis around hotkeys make the menu
>> that much readable, especially when there are lots of short words or
>> even acronyms. Eg :
>> [ ] (U)TS namespace
>> [ ] (I)PC namespace
>> [ ] (U)ser namespace (EXPERIMENTAL)
>> [ ] (P)ID Namespaces (EXPERIMENTAL)
>
> This looks like some ancient stuff and it must be a better way to
> show the short-cuts.

This was the best I could come up with, as ncurses does not let you
use color attributed for menu items. I'll try a different scheme,
where the first capitalized letter is the shortcut. What do u say?

> I would really appreciate if you could look into the above issues.
> If you do a repost with this fixed I will try to do a proper review,
> including code-review next time.

Sure. I'll have a patch ready in a few days.
Cheers.

2008-11-25 08:04:28

by Nir Tzachar

[permalink] [raw]
Subject: Re: [PATCH] ncurses based config V2

Hello.

On Tue, Nov 25, 2008 at 4:33 AM, Roman Zippel <[email protected]> wrote:
> Hi,
>
> On Mon, 24 Nov 2008, Sam Ravnborg wrote:
>
>> > - colors are too dark.
>> Seconded. Please try to find a lighter set of default colors.
>> You may be inspired by one of the menuconfig color-sets.
>>
>> I agree that we could use a lift here - so please do not go for the
>> default color set of menuconfig.
>
> Well, the current colors seems to usable on wide variety of terminals, but
> I'm not colorblind, so I can't really complain. :)
> Before we keep discussing the colors, I'd rather suggest to drop them
> completely and maybe use colors for what actually needs attention.

The idea is using colors to give it a more modern look. Dropping
colors will bring us back to the "dark ages"...

>> > - I'm not convinced that the parenthesis around hotkeys make the menu
>> > that much readable, especially when there are lots of short words or
>> > even acronyms. Eg :
>> > [ ] (U)TS namespace
>> > [ ] (I)PC namespace
>> > [ ] (U)ser namespace (EXPERIMENTAL)
>> > [ ] (P)ID Namespaces (EXPERIMENTAL)
>>
>> This looks like some ancient stuff and it must be a better way to
>> show the short-cuts.
>
> The parenthesis make it really annoying to read...

I agree. See my earlier response to Sam.

>> I would really appreciate if you could look into the above issues.
>> If you do a repost with this fixed I will try to do a proper review,
>> including code-review next time.
>
> The code is rather large, so it might be better to split the code into two
> sources, one for kconfig logic and the other for the interface logic, so
> it becomes easier to read and maintain.

Ok.

Thanks for your interest.
Cheers.

2008-11-25 08:10:40

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH] ncurses based config V2

Hi Nir.

> >> [ ] (U)TS namespace
> >> [ ] (I)PC namespace
> >> [ ] (U)ser namespace (EXPERIMENTAL)
> >> [ ] (P)ID Namespaces (EXPERIMENTAL)
> >
> > This looks like some ancient stuff and it must be a better way to
> > show the short-cuts.
>
> This was the best I could come up with, as ncurses does not let you
> use color attributed for menu items. I'll try a different scheme,
> where the first capitalized letter is the shortcut. What do u say?

Better than the above - let me see it in action and I will judge.
I had preferred something with a colored highlight - but if ncurses
do not support that then I think first catital letter i ok.

Alternative is to use first letter in the prompt and accept the
ambiguity.

> > I would really appreciate if you could look into the above issues.
> > If you do a repost with this fixed I will try to do a proper review,
> > including code-review next time.
>
> Sure. I'll have a patch ready in a few days.

Great - loks forward to see it.

Sam

2008-11-25 12:35:49

by Nir Tzachar

[permalink] [raw]
Subject: Re: [PATCH] ncurses based config V2

Hi.

On Tue, Nov 25, 2008 at 10:11 AM, Sam Ravnborg <[email protected]> wrote:
> Hi Nir.
>
>> >> [ ] (U)TS namespace
>> >> [ ] (I)PC namespace
>> >> [ ] (U)ser namespace (EXPERIMENTAL)
>> >> [ ] (P)ID Namespaces (EXPERIMENTAL)
>> >
>> > This looks like some ancient stuff and it must be a better way to
>> > show the short-cuts.
>>
>> This was the best I could come up with, as ncurses does not let you
>> use color attributed for menu items. I'll try a different scheme,
>> where the first capitalized letter is the shortcut. What do u say?
>
> Better than the above - let me see it in action and I will judge.
> I had preferred something with a colored highlight - but if ncurses
> do not support that then I think first catital letter i ok.
>
> Alternative is to use first letter in the prompt and accept the
> ambiguity.
>

There is too much ambiguity here -- what if the first letter is 'm'?
or 'n'? you have problems then.

>> > I would really appreciate if you could look into the above issues.
>> > If you do a repost with this fixed I will try to do a proper review,
>> > including code-review next time.
>>
>> Sure. I'll have a patch ready in a few days.
>
> Great - loks forward to see it.
>
> Sam
>

2008-11-25 16:03:19

by Roman Zippel

[permalink] [raw]
Subject: Re: [PATCH] ncurses based config V2

Hi,

On Tue, 25 Nov 2008, Nir Tzachar wrote:

> > Before we keep discussing the colors, I'd rather suggest to drop them
> > completely and maybe use colors for what actually needs attention.
>
> The idea is using colors to give it a more modern look. Dropping
> colors will bring us back to the "dark ages"...

After that came the "child ages", where every available color was used
because it was there. :-)
For modern interfaces less is more, so if color is used, it should have a
purpose.

bye, Roman