[Bug 237714] [PATCH] sysutils/xfce4-power-manager: fix craches, improve freebsd support, add DEBUG option

[bugzilla-noreply at freebsd.org]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=237714

Guido Falsi <madpilot at FreeBSD.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|maintainer-feedback?(xfce at F |maintainer-feedback+
                   |reeBSD.org)                 |
                 CC|                            |madpilot at FreeBSD.org
             Status|New                         |In Progress

--- Comment #1 from Guido Falsi <madpilot at FreeBSD.org> ---
Hi,

Thanks for the patch.

I have some questions:

- Why add DEBUG as an option? WITH_DEBUG is not enough to activate it, just
adding --enable-debug as needed when WITH_DEBUG is on.

While there is no strict rule about this and some ports are adding a DEBUG
option, it is not a good practice, and using WITH_DEBUG should be the only way
to enable it. it is not a user flag, it's a developer flag anyway.


- Regarding the source changes, I need some clearifications.

the g_object_ref change should be upstreamed? Have you already sent a bug
report? It looks like a bug to me. Would you propose the patch upstream?

I don't get why you need the "return g_strdup (_("Unknown"));" patch. Looking
at the code I'd say the intention is for that to never be reached, but maybe
there is a bug if 59.0 < value < 60.0. The upstream repository has fixed this
in a different way. I'd be more comfortable importing the newer upstream code
[1], to avoid diverging. Could you modify your patch to match upstream code?


Fir the xfpm-backlight-helper.c patch, before committing it I'd like to know
where the "8" number comes from, and exactly what the patch fixes.

This last hunk also contains some gratuitous changes which would make
upstreaming it difficult. You should not make whitespace changes (they can be
there for upstream code style rules) and I'd also avoid changing the return
logic, and leave it matching what the upstream used. This is because this kind
of change could be refused by upstream if proposed.

Also, while upstream allocating only one gchar looks definitely too little, 64
is definitely too much, since your code never uses more than 5 (including
terminating \0 ). The second argument to g_snprintf could also be calculated
correctly.

As before, should this be upstreamed? Are you wiling to propose it in an
upstream bug report?


[1]
https://github.com/xfce-mirror/xfce4-power-manager/blob/xfce-4.12/settings/xfpm-settings.c#L1567

-- 
You are receiving this mail because:
You are the assignee for the bug.