do not implement get_user_input() in DISABLE_NCURSES builds

PATCH In review Sergey Senozhatsky 3 participants 10 messages Aug 19, 2012 – Aug 20, 2012

lore

Share & tools

lore

Sergey Senozhatsky

author

2012-08-19 21:57 UTC #0 lore raw

PATCH 1/1 do not implement get_user_input() in DISABLE_NCURSES builds

do not implement get_user_input() in DISABLE_NCURSES builds

Signed-off-by Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>

---

 src/lib.cpp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

src/lib.cpp +6 -2

index 77e49a2..ddb0b5c 100644
@@ -436,12 +436,16 @@ void process_directory(const char *d_name, callback fn)
 
 int get_user_input(char *buf, unsigned sz)
 {
+	int ret = 0;
+#ifndef DISABLE_NCURSES
 	fflush(stdout);
 	echo();
 	/* Upon successful completion, these functions return OK. Otherwise, they return ERR. */
-	int ret = getnstr(buf, sz);
+	ret = getnstr(buf, sz);
 	noecho();
 	fflush(stdout);
 	/* to distinguish between getnstr error and empty line */
-	return ret || strlen(buf);
+	ret |= strlen(buf);
+#endif
+	return ret;
 }

Arjan van de Ven

2012-08-20 00:40 UTC #1 lore raw

Quoted 1 line · Sergey Senozhatsky

do not implement get_user_input() in DISABLE_NCURSES builds

no offence.. but this ifdef madness needs to stop.

right now, powertop uses ncurses.

either we want to make it optional, but then this stuff needs to get a lot more modular, or we just declare that you need ncurses.
I'm very much leaning towards the later fwiw...

Sergey Senozhatsky

author

2012-08-20 09:49 UTC #2 lore raw

Quoted 11 lines · Arjan van de Ven

Quoted 1 line · Sergey Senozhatsky
do not implement get_user_input() in DISABLE_NCURSES builds
no offence.. but this ifdef madness needs to stop.
right now, powertop uses ncurses.
either we want to make it optional, but then this stuff needs to get a lot more modular, or we just declare that you need ncurses.
I'm very much leaning towards the later fwiw...

I agree, and actually I don't like this patch (ifdef stuff also affects patches I'm currently working on). it's rather hard to tell how many people will be affected by #2 decision, but we can tell the number for #1 case.

DISABLE_NCURSES is not that nasty in overall, since it just disables some application features, comparing to, for example, DISABLE_TRYCATCH, which disables standard language features for very much non-obvious reason for someone with no embedded background.

that said, it'll be nice if we forbid DISABLE_NCURSES, but we'll still have ifdefs in code because of decisions different people made (like no ncurses in some distros or incomplete/buggy programming language support).

-ss

Chris Ferron

2012-08-20 15:18 UTC #3 lore raw

Quoted 19 lines · Sergey Senozhatsky

Quoted 10 lines · Arjan van de Ven
Quoted 1 line · Sergey Senozhatsky
do not implement get_user_input() in DISABLE_NCURSES builds
no offence.. but this ifdef madness needs to stop.
right now, powertop uses ncurses.
either we want to make it optional, but then this stuff needs to get a lot more modular, or we just declare that you need ncurses.
I'm very much leaning towards the later fwiw...
I agree, and actually I don't like this patch (ifdef stuff also affects patches I'm currently working on). it's rather hard to tell how many people will be affected by #2 decision, but we can tell the number for #1 case.
DISABLE_NCURSES is not that nasty in overall, since it just disables some application features, comparing to, for example, DISABLE_TRYCATCH, which disables standard language features for very much non-obvious reason for someone with no embedded background.

Quoted 6 lines

that said, it'll be nice if we forbid DISABLE_NCURSES, but we'll still have ifdefs in code because of decisions different people made (like no ncurses in some distros or incomplete/buggy programming language support).
	-ss

The only reason we allowed DISABLE_NCURSES in the first place, was to support android. If we don't want to support android out of the box, we can start removing the crude and android can patch PowerTOP within their distro if they wanted.

All other ifdef's need to be scrutinized.
-C

Quoted 4 lines

_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop

Arjan van de Ven

2012-08-20 15:21 UTC #4 lore raw

Quoted 3 lines · Chris Ferron

Quoted 1 line
    -ss
The only reason we allowed DISABLE_NCURSES in the first place, was to support android. If we don't want to support android out of the box, we can start removing the crude and android can patch PowerTOP within their distro if they wanted.

..or for an android build one adds ncurses to the local android.. like libpci and libnl are anyway

Chris Ferron

2012-08-20 15:31 UTC #5 lore raw

Quoted 8 lines · Arjan van de Ven

Quoted 3 lines · Chris Ferron
Quoted 1 line
     -ss
The only reason we allowed DISABLE_NCURSES in the first place, was to support android. If we don't want to support android out of the box, we can start removing the crude and android can patch PowerTOP within their distro if they wanted.
..or for an android build one adds ncurses to the local android.. like libpci and libnl are anyway

Yep, another perfectly reasonable option. So lets make ncurses a hard requirement and remove the ifdef's.

Arjan van de Ven

2012-08-20 15:34 UTC #6 lore raw

Quoted 10 lines · Chris Ferron

Quoted 8 lines · Arjan van de Ven
Quoted 3 lines · Chris Ferron
Quoted 1 line
     -ss
The only reason we allowed DISABLE_NCURSES in the first place, was to support android. If we don't want to support android out of the box, we can start removing the crude and android can patch PowerTOP within their distro if they wanted.
..or for an android build one adds ncurses to the local android.. like libpci and libnl are anyway
Yep, another perfectly reasonable option. So lets make ncurses a hard requirement and remove the ifdef's.

it's already a runtime option anyway (with --html and co)

worst case, if someone really cannot stand this, they can contribute a mini "stub" ncurses that we then compile in for those people who can't cope. But that really should be a separate stub (similar to how we do the pevent lib), not a ton of ifdefs in code.

Chris Ferron

2012-08-20 18:17 UTC #7 lore raw

Quoted 7 lines · Arjan van de Ven

Quoted 6 lines · Chris Ferron
Quoted 5 lines · Arjan van de Ven
Quoted 3 lines · Chris Ferron
Quoted 1 line
      -ss
The only reason we allowed DISABLE_NCURSES in the first place, was to support android. If we don't want to support android out of the box, we can start removing the crude and android can patch PowerTOP within their distro if they wanted.

Quoted 9 lines

Quoted 3 lines
Quoted 2 lines
..or for an android build one adds ncurses to the local android.. like libpci and libnl are anyway
Yep, another perfectly reasonable option. So lets make ncurses a hard requirement and remove the ifdef's.
it's already a runtime option anyway (with --html and co)
worst case, if someone really cannot stand this, they can contribute a mini "stub" ncurses that we then compile in for those people who can't cope. But that really should be a separate stub (similar to how we do the pevent lib), not a ton of ifdefs in code.

Sergey good patch but it sounds like the ifdefs for ncurses and others should go. I have fixed this, hopefully this also helps you out. -C

Sergey Senozhatsky

author

2012-08-20 19:39 UTC #8 lore raw

Quoted 11 lines · Chris Ferron

Quoted 10 lines · Arjan van de Ven
Quoted 7 lines · Chris Ferron
Quoted 5 lines · Arjan van de Ven
Quoted 2 lines · Chris Ferron
The only reason we allowed DISABLE_NCURSES in the first place, was to support android. If we don't want to support android out of the box, we can start removing the crude and android can patch PowerTOP within their distro if they wanted.
..or for an android build one adds ncurses to the local android.. like libpci and libnl are anyway
Yep, another perfectly reasonable option. So lets make ncurses a hard requirement and remove the ifdef's.
it's already a runtime option anyway (with --html and co)

Quoted 8 lines

Quoted 4 lines
worst case, if someone really cannot stand this, they can contribute a mini "stub" ncurses that we then compile in for those people who can't cope. But that really should be a separate stub (similar to how we do the pevent lib), not a ton of ifdefs in code.
Sergey good patch but it sounds like the ifdefs for ncurses and others should go. I have fixed this, hopefully this also helps you out.

Good job! I'm happy that you washed away these ifdefs.

-ss

Sergey Senozhatsky

author

2012-08-20 15:31 UTC #9 lore raw

In reply to Chris Ferron

Quoted 19 lines · Chris Ferron

Quoted 19 lines
Quoted 6 lines
right now, powertop uses ncurses.
either we want to make it optional, but then this stuff needs to get a lot more modular, or we just declare that you need ncurses.
I'm very much leaning towards the later fwiw...
I agree, and actually I don't like this patch (ifdef stuff also affects patches I'm currently working on). it's rather hard to tell how many people will be affected by #2 decision, but we can tell the number for #1 case.
DISABLE_NCURSES is not that nasty in overall, since it just disables some application features, comparing to, for example, DISABLE_TRYCATCH, which disables standard language features for very much non-obvious reason for someone with no embedded background.
that said, it'll be nice if we forbid DISABLE_NCURSES, but we'll still have ifdefs in code because of decisions different people made (like no ncurses in some distros or incomplete/buggy programming language support).

Quoted 9 lines

Quoted 1 line
	-ss
The only reason we allowed DISABLE_NCURSES in the first place, was to support android. If we don't want to support android out of the box, we can start removing the crude and android can patch PowerTOP within their distro if they wanted.
All other ifdef's need to be scrutinized.
-C

as far as I concerned, android build require several defines:
-- DISABLE_NCURSES
-- DISABLE_TRYCATCH
-- DISABLE_I18N
-- perhaps, some more

/* patch series by John Mathew */

-ss