Opened 11 years ago

Closed 11 years ago

#3138 closed enhancement (fixed)

appendchaninfo is defined for webif and monitor module, but is also used for logging

Reported by: malakudi Owned by:
Priority: minor Component: General
Severity: low Keywords: appendchaninfo webif monitor log
Cc: Sensitive: no

Description

Reason for enhancement

appendchaninfo is defined for webif and monitor module, but is also used for logging. So, if you don't build monitor or webif module, you can't use the option. It should be allowed to be configured globally. Maybe then remove option from webif or monitor module. Manpages need to be modified too. Attached patch makes the option only global, but feel free to adapt it.

Possible impacts on other features

None as I can tell.

Attachments (1)

appendchaninfo.patch (1.1 KB ) - added by malakudi 11 years ago.
patch for appendchaninfo config to be applied globally

Download all attachments as: .zip

Change History (19)

by malakudi, 11 years ago

Attachment: appendchaninfo.patch added

patch for appendchaninfo config to be applied globally

comment:1 by gf, 11 years ago

That makes sense (we already have users complaining about appendchaninfo in both sections). Historically the option was in the monitor section. But when new config parser was introduced the people started complaining (rightfully) that when monitor module is disabled appendchaninfo was no longer recognized. So then I added the same option is webif and it used the same variable internally.

I like your solution even better.

Lets see what others have to say about this.

Last edited 11 years ago by gf (previous) (diff)

comment:2 by malakudi, 11 years ago

Unless there is requirement for different behaviour in logging/webif/monitor, then it makes sense to only be global. If different behaviour is expected, then we have to change the option completely (mon_appendchaninfo, webif_appendchaninfo, log_appendchaninfo etc).

comment:3 by gf, 11 years ago

Agree on both counts.

comment:4 by ni hao, 11 years ago

NO GLOBAL option!!!!

Please let people select themselves: webif, monitor and log.
Eventually webif/monitor as combined option, because when you want appendchaninfo in webif then it is assumed that is also so in monitor.

So:
Appendchaninfo webif: ON/OFF
Appednchaninfo monitor: ON/OFF
Appendchaninfo logfile: ON/OFF

or

Appendchaninfo webif/monitor: ON/OFF
Appendchaninfo logfile: ON/OFF

comment:5 by FilipeAmadeuO, 11 years ago

One vote for global. No need to add extra variables to oscam...

comment:6 by Deas, 11 years ago

global. nobody will use different options in webif, monitor and logfile. turning it on and off is enough. to make the output more readable it is better to use the ecmfmt variable.

comment:7 by malakudi, 11 years ago

@ni_hao: Please provide situation where you want channel info on monitor/webif but you don't want it on logfile. When is this really needed?

comment:8 by malakudi, 11 years ago

If needed, I can suggest bitwise configuration.
global value
appendchaninfo=1 logfile only
appendchaninfo=2 webif only
appendchaninfo=4 monitor only

Values of 3,5,6,7 (log+web,log+monitor,web+monitor,all)

I can provide such patch if it is needed.

comment:9 by ni hao, 11 years ago

@malakudi: when I am abroard and have to dial-in, then I want to have a quick look (webif) and I prefer not to see appendchaninfo. I do want to have it in logfile, so that I can look to it later (when I am home again) if necessary. However, it is for me not a big issue. I can live with global.

in reply to:  8 comment:10 by FilipeAmadeuO, 11 years ago

Replying to malakudi:

If needed, I can suggest bitwise configuration.
global value
appendchaninfo=1 logfile only
appendchaninfo=2 webif only
appendchaninfo=4 monitor only

Values of 3,5,6,7 (log+web,log+monitor,web+monitor,all)

I can provide such patch if it is needed.

Nice idea :)

comment:11 by Deas, 11 years ago

i don´t think it is necessary to make it that complicated...

comment:12 by Admin, 11 years ago

There's no sense for separate configuration in my eyes (and I also doubt if it's necessary to config this at all):

  • First usage for setting is if the channelname appears in the log (this automatically implies monitor interface as this interface simply takes the log)
  • Second usage is on status page of WebIf where probably everyone wants to see the channelname...

comment:13 by Entity, 11 years ago

In my opinion, this option can be turned on by default with no user configurable setting at all. As the discussion on reducing configuration items is already ongoing this is a perfect example for me.

Just imagine that it had been turned on by default since the beginning, then now nobody would really start a discussion on it. The chan info would simply be there and the users would be happy with the fact.
Whenever you create an user configurable option you will find someone who is absolutely convinced that he needs it or otherwise the whole application will cease to exists and endanger his immortal soul. Even if it is only user out of 1000.

comment:14 by malakudi, 11 years ago

After looking at the code, I agree with Admin. monitor justs takes log from logfile, so it can't be set differently without major changes in the code. Also, current implementation works like global, even if it is defined in two places. So I think my patch is OK, just make it global. Of course we could also remove the option and make it the default. If someone doesn't want to have channel info, just don't use oscam.srvid file.

comment:15 by gf, 11 years ago

oscam have way too many configuration options, I vote to just kill appendchaninfo. malakudi is right, if someone doesn't want channel names its easier to just remove oscam.srvid file.

Currently you have to fill oscam.srvid and than start to wonder why you don't have channel names until you found "appendchaninfo" and enable it. It makes no sense.

comment:16 by Deas, 11 years ago

@gf - did you forgot to commit the removal of the parameter?

comment:17 by gf, 11 years ago

It seems so. Thanks for reminding me.

comment:18 by gf, 11 years ago

Resolution: fixed
Status: newclosed

I think r8546 closes this ticket.

Note: See TracTickets for help on using tickets.