chore: display & xsettings moved to dde-daemon#763
Conversation
display & xsettings moved to dde-daemon Log: display & xsettings moved to dde-daemon pms: TASK-374777
Reviewer's Guide by SourceryThis pull request moves the display and xsettings modules from startdde to dde-daemon. It also includes changes to the Makefile, session/power1/manager.go, and adds a systemd service for brightness refresh task. The display1 and xsettings1 directories were created to manage display and xsettings functionalities respectively. Updated class diagram for Manager in display1classDiagram
class Manager {
-service *dbusutil.Service
-sysBus *dbus.Conn
-sysSigLoop *dbusutil.SignalLoop
-sessionSigLoop *dbusutil.SignalLoop
-dbusDaemon ofdbus.DBus
-sensorProxy dbus.BusObject
-inputDevices inputdevices.InputDevices
-sysDisplay sysdisplay.Display
-xConn *x.Conn
-PropsMu sync.RWMutex
-sysConfig SysRootConfig
-userConfig UserConfig
-userCfgMu sync.Mutex
-recommendScaleFactor float64
-builtinMonitor *Monitor
-builtinMonitorMu sync.Mutex
-candidateBuiltinMonitors []*Monitor
-monitorMap map[uint32]*Monitor
-monitorMapMu sync.Mutex
-mm monitorManager
-debugOpts debugOptions
-redshiftRunner *redshiftRunner
-sessionActive bool
-newSysCfg *SysRootConfig
-cursorShowed bool
-settings *gio.Settings
-monitorsId monitorsId
-monitorsIdMu sync.Mutex
-hasBuiltinMonitor bool
-rotateScreenTimeDelay int32
-setFillModeMu sync.Mutex
-delayApplyTimer *time.Timer
-delayApplyOptions applyOptions
-prevCurrentNumMonitors int
-prevNumMonitors int
-prevNumMonitorsUpdatedAt time.Time
-delaySwitchMode *delaySwitchMode
-applyMu sync.Mutex
-applySaveMu sync.Mutex
-inApply bool
-futureConfig monitorsFutureConfig
-Monitors []dbus.ObjectPath
-CustomIdList []string
-HasChanged bool
-DisplayMode byte
-Brightness map[string]float64
-Touchscreens dxTouchscreens
-TouchscreensV2 dxTouchscreens
-TouchMap map[string]string
-touchscreenMap map[string]touchscreenMapValue
-touchScreenDialogMap map[string]*exec.Cmd
-touchScreenDialogMutex sync.RWMutex
-CurrentCustomId string
-Primary string
-PrimaryRect x.Rectangle
-ScreenWidth uint16
-ScreenHeight uint16
-MaxBacklightBrightness uint32
-gsColorTemperatureMode int32
-gsColorTemperatureManual int32
-unsupportGammaDrmList []string
-drmSupportGamma bool
-customColorTempTimer *time.Timer
-customColorTempFlag bool
+ColorTemperatureEnabled bool
-SupportColorTemperature bool
-colorTemperatureModeOn int32
+ColorTemperatureMode int32
+ColorTemperatureManual int32
-CustomColorTempTimePeriod string
-isVM bool
}
Class diagram for XSManager in xsettings1classDiagram
class XSManager {
- service *dbusutil.Service
- cfgHelper *configHelper
- greeter greeterInterface
- xConn *x.Conn
- PropsMu sync.RWMutex
- scaleFactor float64
- screenWidth uint16
- screenHeight uint16
- Monitors []dbus.ObjectPath
- HasChanged bool
- primary string
- primaryRect x.Rectangle
- sessionActive bool
- settings *gio.Settings
- sysBus *dbus.Conn
- sysSigLoop *dbusutil.SignalLoop
- sessionSigLoop *dbusutil.SignalLoop
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review###PATH:./deepin-startdde-master/README.md deepin-startdde |
|
TAG Bot TAG: 6.1.26 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @fly602 - I've reviewed your changes - here's some feedback:
Overall Comments:
- The removal of
appmanagerfromkeybinding1/manager.goseems like a significant change; ensure that the functionality it provided is no longer needed or has been moved elsewhere. - The addition of
greeter-display-daemonandfix-xauthority-permtoMakefilesuggests new binaries; verify that these are correctly installed and configured.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 4 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| v, ok := value.Value().(float64) | ||
| if !ok { | ||
| // TODO: dconfig float64的类型处理有问题，待该问题解决后去掉该代码 |
There was a problem hiding this comment.
suggestion: Revisit fallback conversion for float64 in GetDouble.
The function GetDouble uses a fallback conversion from int64 to float64 when the value isn’t a float64. Consider a more robust strategy or clearer error handling once the underlying type issue is resolved.
Suggested implementation:
switch vTyped := value.Value().(type) {
case float64:
return vTyped
case int64:
logger.Warningf("key %v type was int64, converted to float64", key)
return float64(vTyped)
default:
logger.Warningf("key %v has unsupported type %T", key, vTyped)
return -1
}This change refactors the conversion logic within GetDouble to use a type switch, providing clearer handling for unsupported types. Review your logging and error handling strategy to ensure it conforms with the rest of your codebase.
| return false | ||
| } | ||
|
|
||
| func GetRecommendedScaleFactor(x *x.Conn) float64 { |
There was a problem hiding this comment.
suggestion: Use consistent logging instead of fmt.Printf in screenscale functions.
There are several fmt.Printf calls used for error reporting and debug output. It would be preferable to use the logger instance for consistency with the rest of the code base.
Suggested implementation:
logger.Printf("randr version %d.%d", randrVersion.ServerMajorVersion, randrVersion.ServerMinorVersion)• Verify that a logger instance (or package) is imported and used consistently throughout the file. For example, if not already present, add an import statement for the logger package at the top of the file.
• Replace any other fmt.Printf calls in the screenscale functions with logger calls following the same pattern.
| } | ||
| } | ||
|
|
||
| func (cfg *UserConfig) updateUuid(monitors Monitors) (changed bool) { |
There was a problem hiding this comment.
issue (complexity): Consider creating a helper function to consolidate the duplicated logic in the updateUuid functions for SysConfig and UserConfig to reduce code duplication and improve readability .
Consider consolidating your duplicated “updateUuid” loops into a single helper to reduce nesting and repetition. For example, you can extract the common logic of iterating over the screens map and updating keys into a generic helper. Here’s a small sketch:
```go
// A generic helper to update UUID keys for screen configs.
func updateScreenConfigs[V any](screens map[string]V, monitors Monitors, updateFunc func(V, Monitors) (V, bool)) bool {
changed := false
additional := make(map[string]V)
for key, cfg := range screens {
uuidParts := strings.Split(key, monitorsIdDelimiter)
var partMonitors Monitors
for _, uuid := range uuidParts {
if monitor := monitors.GetByUuid(uuid); monitor != nil {
partMonitors = append(partMonitors, monitor)
}
}
if len(uuidParts) == len(partMonitors) {
newKey := partMonitors.getMonitorsId().v1
updatedCfg, cfgChanged := updateFunc(cfg, monitors)
if newKey != key || cfgChanged {
additional[newKey] = updatedCfg
delete(screens, key)
changed = true
}
}
}
for key, cfg := range additional {
screens[key] = cfg
}
return changed
}Now, both the SysConfig and UserConfig update functions can call this helper with appropriate update functions (e.g., a version that works on *SysScreenConfig or UserScreenConfig). This removes duplicated looping logic while preserving functionality and makes the code easier to follow.
| } | ||
| } | ||
|
|
||
| func writeXSInfoHeader(writer io.Writer, header *xsItemHeader) { |
There was a problem hiding this comment.
issue (complexity): Consider using a helper function to write multiple fields at once to reduce boilerplate code in serialization functions like writeXSInfoHeader.
Consider reducing the repeated manual write calls by using a helper that accepts a list of fields. This can simplify your serialization routines while keeping the logic intact. For instance, you could introduce a function:
func writeFields(writer io.Writer, fields ...interface{}) {
for _, field := range fields {
if err := binary.Write(writer, defaultByteOrder, field); err != nil {
logger.Warning(err)
}
}
}Then, replace functions like writeXSInfoHeader with a more concise approach:
func writeXSInfoHeader(writer io.Writer, header *xsItemHeader) {
writeFields(writer,
header.sType,
uint8(0), // skip 1 byte
header.nameLen,
[]byte(header.name),
make([]byte, pad(int(header.nameLen))),
header.lastChangeSerial,
)
}This change keeps all functionality but reduces low-level boilerplate calls.
| return true | ||
| } | ||
|
|
||
| func (gc *GSConfig) GetString(key string) string { |
There was a problem hiding this comment.
issue (complexity): Consider using a generic helper function to abstract the key check and value retrieval/setting logic in the Get and Set methods.
You can reduce the repeated key-checking logic by abstracting the “check key then call” pattern into a helper function. For example, if you’re using Go 1.18+ you can use generics to create a unified getter. The following example shows how you might reduce duplication for the getters:
func (gc *GSConfig) safeGet[T any](key string, defaultValue T, getter func(string) T) T {
if !gc.hasKey(key) {
return defaultValue
}
return getter(key)
}
// Usage:
func (gc *GSConfig) GetString(key string) string {
return gc.safeGet(key, "", gc.gs.GetString)
}
func (gc *GSConfig) GetInt(key string) int32 {
return gc.safeGet(key, -1, gc.gs.GetInt)
}
func (gc *GSConfig) GetBoolean(key string) bool {
return gc.safeGet(key, false, gc.gs.GetBoolean)
}
func (gc *GSConfig) GetDouble(key string) float64 {
return gc.safeGet(key, -1, gc.gs.GetDouble)
}A similar helper could be created for setters (if the return types are consistent), for example:
func (gc *GSConfig) safeSet[T any](key string, value T, setter func(string, T) bool) bool {
if !gc.hasKey(key) {
return false
}
return setter(key, value)
}
// Usage:
func (gc *GSConfig) SetString(key string, value string) bool {
return gc.safeSet(key, value, gc.gs.SetString)
}
func (gc *GSConfig) SetInt(key string, value int32) bool {
return gc.safeSet(key, value, gc.gs.SetInt)
}
func (gc *GSConfig) SetBoolean(key string, value bool) bool {
return gc.safeSet(key, value, gc.gs.SetBoolean)
}
func (gc *GSConfig) SetDouble(key string, value float64) bool {
return gc.safeSet(key, value, gc.gs.SetDouble)
}These small focused changes preserve all existing functionality while reducing duplication and complexity.
| return configs, nil | ||
| } | ||
|
|
||
| func setFirefoxDPI(value float64, src, dest string) error { |
There was a problem hiding this comment.
issue (complexity): Consider using a regular expression to simplify the line and string matching logic in setFirefoxDPI to improve readability and reduce potential bugs related to manual string handling, such as incorrect DPI settings or errors when appending new settings.
Consider replacing the manual line‐and‐string matching in `setFirefoxDPI` with a regular expression. This can simplify the logic, make it easier to read, and help avoid subtle bugs. For example, you could refactor the function like so:
```go
import (
"fmt"
"os"
"regexp"
"strings"
)
func setFirefoxDPI(value float64, src, dest string) error {
contents, err := os.ReadFile(src)
if err != nil {
return err
}
// Build the new user_pref line:
target := fmt.Sprintf(`user_pref("layout.css.devPixelsPerPx", "%.2f");`, value)
// Regular expression to match the existing dpi setting.
re := regexp.MustCompile(`user_pref\("layout\.css\.devPixelsPerPx",\s*(?:"-?\d+(?:\.\d+)?"|-?\d+(?:\.\d+)?)(\));`)
text := string(contents)
if re.MatchString(text) {
// If value matches, and it's already set correctly, return early.
if strings.Contains(text, target) {
return nil
}
// Replace the old line with the target.
text = re.ReplaceAllString(text, target)
} else {
// If not found and value is not -1, append the new setting.
if value == -1 {
return nil
}
text += "\n" + target
}
return os.WriteFile(dest, []byte(text), 0644)
}Actionable steps:
- Import the
regexppackage. - Define a regex that matches the current Firefox DPI setting.
- Use
ReplaceAllStringto update the setting if found. - Append the setting if the pattern isn’t present and the DPI isn’t -1.
This approach maintains the functionality while reducing the complexity of manual string and line handling.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602, yixinshark The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
display & xsettings moved to dde-daemon
Log: display & xsettings moved to dde-daemon
pms: TASK-374777
Summary by Sourcery
Move display and xsettings modules from startdde to dde-daemon
New Features:
Chores: