New libqsgepaper and initial rMPP support#395
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request updates several components related to display and device settings. In the display server, the rectangle data used for GUI updates now comes from an auxiliary buffer rather than the main framebuffer, and framebuffer update methods have been simplified. The EPAPER feature has been removed from the main configuration. Additionally, dynamic screen dimensions are now used across multiple modules, and support for a new device type ("reMarkable Paper Pro") has been added. The ePaper library’s build configuration and EPFramebuffer API have been restructured to accommodate these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant DBusInterface
participant GUIThread
participant EPFramebuffer
participant Driver
%% Display update flow initiated from a DBus event
DBusInterface->>GUIThread: Trigger exitExclusiveMode/exclusiveModeRepaint
GUIThread->>EPFramebuffer: Retrieve auxBuffer rectangle data
EPFramebuffer-->>GUIThread: Return rectangle dimensions
GUIThread->>Driver: Call swapBuffers(Color, QualityMode, CompleteRefresh)
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
asivery
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
shared/liboxide/devicesettings.cpp (1)
167-172: Fix inconsistent indentation.The indentation in the
getTouchHeight()method is inconsistent after adding the RMPP case. Thedefault:case and subsequent lines should be aligned with the other cases.case DeviceType::RMPP: return 2832; - default: - return 0; - } - } + default: + return 0; + } + }shared/liboxide/eventfilter.cpp (1)
14-15: Consider replacing macros with inline functions.
Using macros forDISPLAYWIDTHandDISPLAYHEIGHTcan reduce type safety and hinder debugging. Inline functions or static constants can serve the same purpose while offering clearer semantics.-#define DISPLAYWIDTH (deviceSettings.getScreenWidth()) -#define DISPLAYHEIGHT (deviceSettings.getScreenHeight()) +static inline int displayWidth() { + return deviceSettings.getScreenWidth(); +} +static inline int displayHeight() { + return deviceSettings.getScreenHeight(); +}applications/display-server/guithread.cpp (1)
76-78: Offer to help with memfd_create implementation.
The TODO suggests a plan to switch from/dev/nullto a dynamically created file descriptor. Let me know if you'd like assistance implementing this feature.shared/epaper/epframebuffer.h (4)
42-42: Factory-method pointer return.
Returning a raw pointer is typical in singletons/factory methods, but consider returning by reference or using a smart pointer if you need stronger lifetime guarantees.
47-48: Add documentation for newly introduced methods.
Theseinitialize()andsync()methods are self-explanatory, but additional doc comments clarifying their usage and preconditions might help maintainability.
52-68: Check layout consistency inEPFramebufferAcep2.
This class relies on carefully placed buffers and the size ofQImage. Changes in Qt versions or compilers may cause alignment or size-differences, leading to potential memory corruption.If you’d like, I can provide a shell script to search for any references that might incorrectly cast or offset these buffer members.
70-85: Ensure robust handling ofEPFramebufferFusionmemory layout.
Similar toEPFramebufferAcep2, this class uses partially opaque data and members relying on specific offset sizes. Confirm thatsizeof(QImage)is stable on ARM builds to avoid out-of-bounds access.Would you like me to generate a web search query to investigate
QImagesize consistency across Qt builds?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
shared/epaper/libqsgepaper.sois excluded by!**/*.so
📒 Files selected for processing (13)
applications/display-server/dbusinterface.cpp(2 hunks)applications/display-server/guithread.cpp(5 hunks)applications/display-server/main.cpp(0 hunks)applications/settings-manager/main.cpp(1 hunks)applications/system-service/apibase.cpp(1 hunks)applications/system-service/screenapi.h(1 hunks)shared/epaper/epaper.pro(1 hunks)shared/epaper/epframebuffer.h(3 hunks)shared/liboxide/devicesettings.cpp(4 hunks)shared/liboxide/devicesettings.h(2 hunks)shared/liboxide/eventfilter.cpp(1 hunks)shared/qpa/oxideeventfilter.cpp(2 hunks)shared/qpa/oxideintegration.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- applications/display-server/main.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (22)
shared/liboxide/devicesettings.h (2)
29-30: LGTM: Device type enum updated correctly.The addition of the RMPP device type to the enum with proper documentation looks good.
78-87: LGTM: New screen dimension methods well documented.The new methods for retrieving screen dimensions are properly documented and follow the existing code style pattern.
applications/system-service/screenapi.h (1)
20-23: Good improvement: Dynamic screen dimensions.Replacing hardcoded screen dimensions with dynamic values from deviceSettings improves flexibility and allows for better support of different device types.
shared/qpa/oxideeventfilter.cpp (2)
9-9: LGTM: Required header added.Adding the deviceSettings header is appropriate for accessing the screen dimension methods.
88-89: Good improvement: Dynamic screen dimensions.Replacing hardcoded dimensions with dynamic values from deviceSettings enhances device compatibility and maintainability.
shared/liboxide/devicesettings.cpp (5)
128-129: LGTM: Device name added appropriately.Adding the proper display name for the new device type is good.
154-155: LGTM: Touch dimensions added for new device.Touch input dimensions added correctly for the RMPP device.
174-196: LGTM: Screen dimension methods implemented correctly.The implementations for
getScreenWidth()andgetScreenHeight()look good, providing appropriate values for each device type.
106-110:Details
❓ Verification inconclusive
Verify device identification method.
Using "reMarkable Ferrari" to identify the RMPP device seems like a development codename rather than a production identifier. Consider if this is the final identification string that will be used in production.
🏁 Script executed:
#!/bin/bash # Check if there are any references to the final device name in the codebase echo "Searching for references to reMarkable Ferrari and other device identifiers..." rg -A 2 -B 2 "reMarkable Ferrari" --glob "!*.md"Length of output: 563
Action: Confirm the Device Identifier
In
shared/liboxide/devicesettings.cpp(lines 106–110), the code checks if the model name contains"reMarkable Ferrari". This string appears to be a development codename rather than a finalized production identifier for the RMPP device. Please verify whether"reMarkable Ferrari"is indeed the intended identifier for production or if it should be updated to reflect the correct final device name.
141-142:Details
❓ Verification inconclusive
Verify touch environment setting for RMPP.
The RMPP device has an empty touch environment setting. Confirm if this is intentional or if specific settings are needed for proper touch input orientation.
🏁 Script executed:
#!/bin/bash # Look for any rotation or inversion settings for RMPP elsewhere in the codebase echo "Searching for touch orientation settings for RMPP..." rg -A 3 -B 3 "RMPP.*rotate|rotate.*RMPP" --glob "!*.md" rg -A 3 -B 3 "touchscreen.*orientation|orientation.*touchscreen" --glob "!*.md"Length of output: 254
I'll need to gather a bit more context before finalizing the review comment. Could you please run an additional verification script to further search for any RMPP-specific touch or orientation configurations in the codebase? For example, a script that searches for general mentions of "RMPP" and entries that might involve "touch" or "orientation" might shed more light on whether the empty setting for RMPP is intentional.
Below is a proposed script for re-verification:
Once you have the output from this script, we can better determine if the empty touch environment setting for the RMPP device is deliberate or if further adjustments are needed.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for any mention of RMPP in the codebase..." rg "RMPP" --glob "!*.md" echo "Searching for RMPP touch related settings..." rg -A 5 -B 5 "RMPP.*touch" --glob "!*.md"Length of output: 1060
Action Required: Verify RMPP Touch Configuration
After searching the codebase, we found several references to
RMPPinshared/liboxide/devicesettings.cppandshared/liboxide/devicesettings.h, but no evidence of any dedicated touch orientation or inversion settings for this device type. This suggests that returning an empty string for the RMPP touch environment might be intentional.
- The code snippet in question:
case DeviceType::RMPP: return "";- Our searches for keywords like
rotate,touch, andorientationin relation to RMPP did not reveal any additional configurations.- Next Step: Please verify against the product specifications or with the relevant stakeholders whether the empty touch environment setting for RMPP is by design or if specific parameters (e.g., rotation or inversion settings) should be applied.
shared/liboxide/eventfilter.cpp (1)
12-12: No issues with new include.
This header inclusion is necessary for accessingdeviceSettings.applications/display-server/guithread.cpp (4)
64-64: Validate auxiliary buffer geometry.
Before creatingGUIThreadwithauxBuffer.rect(), consider handling edge cases where the auxiliary buffer might not be initialized or has a zero-size rectangle. This helps prevent potential runtime errors.
79-79: Ensure correct usage of /dev/null.
Opening/dev/nullis likely a temporary placeholder and won't provide meaningful read/write operations for a framebuffer. Confirm that this usage is intentional and doesn't break any downstream logic.
179-185: Clear function logic looks consistent.
Clearing the auxiliary buffer with white and issuing a complete refresh appears correct. Verify whether additional error handling is needed forswapBuffers()failures.
230-230: Aux buffer usage confirmed.
Switching toauxBufferis aligned with the new approach. No issues here.shared/epaper/epaper.pro (4)
4-4: TEMPLATE changed to aux.
Switching fromlibtoauxis unorthodox. Ensure this aligns with the intended build artifacts and doesn't break downstream usage.
6-7: Copying .so and header together could be fragile.
Copying the library file in the same command as the header may cause unexpected issues if either path changes or if the shared object isn't built yet. Confirm build order or consider separate targets.
11-11: Installing multiple files.
Including both the header and the.soinepframebuffer_h_install.filesbroadens deployment scope. Double-check that this matches your packaging requirements and doesn't omit versioned library handling.
16-17: Review new dependencies and clean rules.
PRE_TARGETDEPSandQMAKE_CLEANnow include theraw_copytarget. Ensure these additions align with your build steps and that cleaning removes all generated artifacts.shared/epaper/epframebuffer.h (3)
11-13: Validate newly introduced macros for reMarkable 2.
These macros are derived from disassembly and may change in future library updates. Consider maintaining them in a single source of truth or verifying them dynamically to avoid misalignment.If you’d like, I can provide a shell script or a web search query to confirm these offsets match the actual structure size.
15-19: Confirm architecture detection logic.
The definition ofDEFAULT_EPFR_RETURNdiffers across__aarch64__and the fallback. Ensure that reMarkable 2 builds consistently match the intended macro.
40-40: Signature looks good.
The revised parameter list and default argument appear consistent with usage patterns. No issues spotted.
|
|
||
| epframebuffer_h.target = epframebuffer.h | ||
| epframebuffer_h.commands = cp $$PWD/epframebuffer.h $$OUT_PWD | ||
| epframebuffer_h.target = raw_copy |
There was a problem hiding this comment.
I don't see raw_copy defined here. Shouldn't it be what was here before?
|
@asivery I just merged the c abi branch, luckily there are no conflicts! |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…, fix some crashes.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…, fix crash, only draw changed region for exclusive paint
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/system-service/appsapi.cpp (1)
1090-1103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSnapshot
applicationsbefore stopping them during shutdown.
stopNoSecurityCheck()can synchronously unregister transient apps, so iterating the liveapplicationsmap here can mutate the container mid-loop. That risks skipping apps or walking entries that were just removed.Suggested fix
- for (auto app : applications) { + const auto apps = applications.values(); + for (auto app : apps) { if (app->stateNoSecurityCheck() != Application::Inactive) { auto text = "Stopping " + app->displayName() + "..."; O_DEBUG(text.toStdString().c_str()); notification->setProperty("text", text); } app->stopNoSecurityCheck(); } O_INFO("Ensuring all applications have stopped..."); - for (auto app : applications) { + for (auto app : apps) { app->waitForFinished(); app->deleteLater(); } applications.clear();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@applications/system-service/appsapi.cpp` around lines 1090 - 1103, Create a snapshot of the current applications container before calling stopNoSecurityCheck to avoid mutating the live map during iteration: copy the entries from applications into a local vector (e.g., appsToStop), iterate appsToStop to call stateNoSecurityCheck(), set the notification text and call stopNoSecurityCheck() on each item, and then iterate the original applications collection to call waitForFinished() and deleteLater() and finally clear() applications; reference: applications, appsToStop (new), stopNoSecurityCheck(), stateNoSecurityCheck(), waitForFinished(), deleteLater().
♻️ Duplicate comments (2)
.github/workflows/publish.yml (1)
19-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
persist-credentials: falseto prevent credential leakage.The checkout step does not set
persist-credentials: false, allowing GITHUB_TOKEN to persist in local git config. This could leak credentials through uploaded artifacts (line 25).🔒 Suggested fix
- name: Checkout the Git repository uses: actions/checkout@v6 with: + persist-credentials: false submodules: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yml around lines 19 - 21, The checkout action currently uses actions/checkout@v6 with submodules enabled but doesn’t disable credential persistence; update the checkout step (the actions/checkout@v6 usage) to add persist-credentials: false so the GITHUB_TOKEN is not written to the local git config and cannot be leaked via artifacts or subsequent git operations.applications/launcher/main.cpp (1)
31-34:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep signal-handler path async-signal-safe only.
signalHandler2still routes into code that calls Qt APIs (QTimer::singleShot, signal emission), which is undefined in a POSIX signal context.#!/bin/bash # Verify non-async-signal-safe Qt work is reachable from the signal handler path. rg -n -C4 'signalHandler2|shutdown_handler|QTimer::singleShot|emit controller->reload|signal\s*\(\s*SIGCONT' applications/launcher/main.cppAlso applies to: 98-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@applications/launcher/main.cpp` around lines 31 - 34, signalHandler2 currently calls shutdown_handler which invokes Qt APIs (e.g., QTimer::singleShot and controller->reload) from a POSIX signal context; replace the direct Qt calls from the signal path with an async-signal-safe notification mechanism: set a sig_atomic_t flag or write a byte to a self-pipe in signalHandler2, and change shutdown_handler (or a new poll/slot in main/Qt event loop) to perform the actual Qt work (QTimer::singleShot, emit controller->reload) when the main loop notices the flag or reads the pipe; ensure signalHandler2, any SIGCONT handler, and related handlers touch only async-signal-safe globals/functions.
🧹 Nitpick comments (1)
applications/system-service/dbusservice.cpp (1)
278-283: ⚡ Quick winKeep
dependantsunique per DBus service.
releaseAPI()andserviceOwnerChanged()both clear all entries for a service, so this list is effectively treated as a set. Appending unconditionally here lets one client accumulate duplicates until teardown.Suggested fix
- api.dependants->append(message.service()); + if (!api.dependants->contains(message.service())) { + api.dependants->append(message.service()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@applications/system-service/dbusservice.cpp` around lines 278 - 283, The dependants list for each API (api.dependants) must act like a set, so don’t append the same DBus service multiple times; update the logic in the block that currently does api.dependants->append(message.service()) to first check whether message.service() is already present (e.g., using contains() or std::find) and only append if it isn’t, ensuring api.dependants remains unique per DBus service (this affects the code paths around serviceOwnerChanged() and releaseAPI() which clear entries).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/publish.yml:
- Line 19: The workflow uses floating action tags (actions/checkout@v6,
actions/upload-pages-artifact@v3, actions/deploy-pages@v4) which should be
pinned to immutable commit SHAs; update each `uses:` entry to the corresponding
action's full commit SHA (replace `@vX` with @<commit-sha>) so the workflow is
deterministic and supply-chain secure, ensuring you fetch the canonical SHA from
the action's GitHub repo releases/tags and substitute it for
actions/checkout@v6, actions/upload-pages-artifact@v3, and
actions/deploy-pages@v4 respectively.
In `@applications/desktop-file-install/main.cpp`:
- Around line 113-120: Check the result of toFile.write(...) and toFile.flush()
after opening and before closing the file: call toFile.write() and verify the
returned qint64 equals the JSON byte length (json.toUtf8().size()), then call
toFile.flush() and ensure it returns true; on any mismatch or failure, log an
error including path, set success = false and continue, then close the file.
Locate the block around the variables json, toFile, path and the calls to
toFile.open(), toFile.write(), and toFile.close() and add these checks and error
handling for short-writes/flush failures.
In `@applications/display-server/connection.cpp`:
- Around line 63-80: Connection constructor must abort when either socketpair()
fails: detect failure of the first or second ::socketpair call and do not
continue initializing with -1 fds; close any fds that were successfully created
(m_clientFd/m_serverFd or m_clientInputFd/m_serverInputFd) and clean up
m_notifier if already allocated, then throw an exception (e.g.,
std::runtime_error) to abort construction. Specifically, after the first
::socketpair call, if it fails, ensure you close any non -1 entries in fds and
throw; after creating m_notifier, if the second ::socketpair fails, close the
previously created m_clientFd/m_serverFd and any fds from the failed call before
throwing; alternatively introduce a bool valid flag set only after both
socketpairs succeed and only construct m_notifier after both succeed—use
function names/fields socketpair, m_clientFd, m_serverFd, m_clientInputFd,
m_serverInputFd, m_notifier, and Connection::readSocket to locate and implement
the fix.
In `@applications/display-server/dbusinterface.cpp`:
- Around line 677-680: Read m_focused into a local variable before calling
Oxide::dispatchToThread and capture that local in the lambda so the dispatched
work uses the stable pointer; specifically, in the branch that checks m_focused,
assign e.g. auto focused = m_focused, then call
Oxide::dispatchToThread(focused->thread(), [this, focused, device, events] { if
(focused) focused->inputEvents(device, events); }); to avoid TOCTOU where
m_focused could become nullptr or change target between scheduling and
execution.
- Around line 780-791: The range-loop holds ptr as a reference into connections
and then unlocks connectionsLock before calling connections.removeAll(ptr),
which makes ptr invalid if the list changes; to fix, copy the value out before
unlocking (e.g., auto localPtr = ptr) and use localPtr with
connections.removeAll(localPtr) after acquiring the write lock, or rewrite the
loop to use an index/iterator that is only used while the lock is held; update
the code around connectionsLock, the loop over connections, ptr, and the call to
guiThread->notify() accordingly.
In `@applications/display-server/evdevdevice.cpp`:
- Around line 157-159: In EvDevDevice::emitSomeEvents() the reverse-iterator
arithmetic using (i - 1) is invalid when i == events.rbegin(); replace the
current computation of index that uses std::next(i - 1).base() with one that
uses i.base() (or adjust to std::next(i).base() as needed) so you obtain the
correct forward iterator for slicing events; update the code that builds the
vector (some) to use events.begin() up to that i.base()-derived iterator and
remove the i-1 usage to avoid undefined behavior when checking
EV_SYN/SYN_REPORT.
In `@applications/display-server/guithread.cpp`:
- Around line 312-340: GUIThread::sendUpdate currently drops the client-supplied
marker so Connection::Wait (which issues MXCFB_WAIT_FOR_UPDATE_COMPLETE with
mxcfb_update_marker_data{marker,0}) can block on the wrong value; fix by
propagating the marker through the framebuffer submission path: add a marker
parameter to EPFramebuffer::swapBuffers (or an overload) and have
GUIThread::sendUpdate pass its marker through, ensuring EPFramebuffer
stores/returns the same marker value associated with the submitted update (or
maps the client marker to the internal update id and returns that mapping) so
Connection::Wait uses the identical marker produced/recognized by EPFramebuffer;
update all callers of swapBuffers (and relevant types/signatures) to
accept/forward the marker so the MXCFB_WAIT_FOR_UPDATE_COMPLETE path waits on
the correct update.
In `@applications/launcher/controller.h`:
- Line 882: The WiFi state string is misspelled causing QML state logic to fail;
locate the call ui->setProperty("state", "unkown") and change the property value
to the correct spelling "unknown" so state comparisons in QML match expected
values (update any other occurrences of "unkown" in controller.h or related code
paths).
- Around line 775-776: The charger-state branch is erroneously comparing the
charger state variable "state" against BatteryNotPresent; update the check to
use the correct enum ChargerNotPresent (i.e., replace comparisons of
BatteryNotPresent with ChargerNotPresent) where the UI property is set (the
block that calls ui->setProperty("present", true)) so charger-present logic uses
the ChargerNotPresent enum consistently.
- Around line 634-645: The branch that handles notifications->length() > 1
returns early after calling setNotification(...) which prevents emitting
notificationsChanged and causes observers to miss updates; remove the early
return in the >1 branch (the block that calls setNotification(QStringLiteral("%1
notifications").arg(notifications->length()))) and ensure emit
notificationsChanged(notifications) runs for all branches (i.e., after the
if/else ladder), keeping the existing setNotification, clearNotification, and
notificationsChanged calls intact for consistency.
In `@applications/launcher/wifinetworklist.h`:
- Around line 345-356: The removal loop in WifiNetworkList::remove(const
QDBusObjectPath& path) deletes WifiNetwork instances from the networks list
without emitting model change notifications; wrap each removal with
QAbstractListModel notifications by calling beginRemoveRows(parent, row, row)
immediately before removing the element (use the current index of the
iterator/network in networks) and endRemoveRows() immediately after
i.remove()/delete network, or detect and remove consecutive ranges and call
beginRemoveRows/endRemoveRows once per range so the model contract is preserved.
- Around line 216-238: The append(QList<Network*> networks) currently removes
elements from the incoming QList while range-iterating
(networks.removeAll(network)), which can invalidate iteration and later produces
invalid beginInsertRows ranges when nothing remains; update
append(QList<Network*> networks) to first build a separate QList<Network*>
toInsert: iterate the incoming networks without mutating it, for each network
compare ssid() against existing entries in this->networks (calling
item->setNetwork(network) when matched) and otherwise append to toInsert; if
toInsert is empty skip beginInsertRows/endInsertRows and sort(), otherwise call
beginInsertRows with this->networks.length() and this->networks.length() +
toInsert.length() - 1, append new WifiNetwork(...) objects for each entry in
toInsert, then endInsertRows and sort(); apply the identical
collect-and-skip-empty-insert pattern to append(QList<BSS*> bsss) (the BSS path
at ~lines 257-269), avoiding in-loop removals and guarding beginInsertRows
against empty ranges.
- Around line 313-334: The comparator used for sorting WifiNetwork objects
violates strict weak ordering because it unconditionally returns true when
a->connected() is true without checking b->connected(); update the lambda
comparator in wifinetworklist.h to compare the boolean flags explicitly (e.g.,
compare a->connected() vs b->connected() first using a != b -> return
a->connected(); then do the same for available and known) and only use ssid() as
a final tie-breaker so comparisons are deterministic and antisymmetric for pairs
like two connected networks.
- Around line 271-280: The clear() implementation must avoid calling
beginRemoveRows/endRemoveRows when networks is empty and must pass an inclusive
end index of networks.length() - 1 (not networks.length()); modify clear() to
first check if networks.isEmpty() (or networks.length() == 0) and return early,
otherwise call beginRemoveRows(QModelIndex(), 0, networks.length() - 1), then
delete entries in networks, clear the container, and finally call
endRemoveRows(); reference the clear() method and the networks container as the
targets to change, and keep beginRemoveRows/endRemoveRows paired correctly.
- Around line 378-379: WifiNetworkList's QML-exposed methods dereference the
possibly-null member api (methods scanning() and scan(bool)), so add null
guards: in bool WifiNetworkList::scanning() return false if api is nullptr, and
in Q_INVOKABLE void WifiNetworkList::scan(bool active) early-return (no-op) if
api is nullptr instead of calling api->scan(active).waitForFinished();;
reference the WifiNetworkList class and its api member and update scanning() and
scan(bool) to check api before use (optionally log or warn when api is null).
In `@applications/system-service/apibase.cpp`:
- Around line 55-91: APIBase::getFrameBuffer() currently allocates file = new
QFile() before DBus/dup() and uses QFile::open(fd, QFile::ReadWrite) which by
default keeps the duplicated fd open on QFile deletion, causing leaked QFile
objects on early returns and leaked fds after open failures; fix by deferring
construction of the QFile until after you have a valid duplicated fd (after
dup(qfd.fileDescriptor()) succeeds) or ensure every early-return path deletes
the previously-allocated QFile, and when opening with file->open(fd, ...) set
the QFile handle ownership flag so the QFile will close the underlying fd on
close/delete (i.e. use the QFile open variant/flag that makes QFile take
ownership), and also ensure that if file->map() or subsequent QImage
construction fails you explicitly close the fd (or rely on QFile ownership)
before deleting file to avoid descriptor leaks.
In `@applications/system-service/application.cpp`:
- Around line 91-93: The conditional incorrectly negates only setUser and not
setGroup causing successful setGroup to trigger the warning; change the check so
it fails when either m_process->setUser(user()) or m_process->setGroup(group())
returns false (i.e., if (!m_process->setUser(user()) ||
!m_process->setGroup(group())) ), so the O_WARNING(("Failed to set user/group
for the process")) is executed on any failure and the child is prevented from
continuing; update the condition around m_process->setUser, m_process->setGroup,
user(), group() and ensure the failure branch still handles cleanup/abort as
intended.
In `@applications/system-service/main.cpp`:
- Around line 138-142: Before calling stopProcess on PIDs from
Oxide::lsof(lockPath), verify each PID is actually the tarnish process by
checking identifying attributes (e.g., process name/argv or executable path)
rather than blindly trusting Oxide::lsof output; update the loop that iterates
over Oxide::lsof(lockPath) to, for each lockingPid, call
Oxide::processExists(lockingPid) and then confirm the process identity (match
process name "tarnish" or compare its executable path/argv to the current
tarnish binary) and only then call stopProcess(lockingPid).
---
Outside diff comments:
In `@applications/system-service/appsapi.cpp`:
- Around line 1090-1103: Create a snapshot of the current applications container
before calling stopNoSecurityCheck to avoid mutating the live map during
iteration: copy the entries from applications into a local vector (e.g.,
appsToStop), iterate appsToStop to call stateNoSecurityCheck(), set the
notification text and call stopNoSecurityCheck() on each item, and then iterate
the original applications collection to call waitForFinished() and deleteLater()
and finally clear() applications; reference: applications, appsToStop (new),
stopNoSecurityCheck(), stateNoSecurityCheck(), waitForFinished(), deleteLater().
---
Duplicate comments:
In @.github/workflows/publish.yml:
- Around line 19-21: The checkout action currently uses actions/checkout@v6 with
submodules enabled but doesn’t disable credential persistence; update the
checkout step (the actions/checkout@v6 usage) to add persist-credentials: false
so the GITHUB_TOKEN is not written to the local git config and cannot be leaked
via artifacts or subsequent git operations.
In `@applications/launcher/main.cpp`:
- Around line 31-34: signalHandler2 currently calls shutdown_handler which
invokes Qt APIs (e.g., QTimer::singleShot and controller->reload) from a POSIX
signal context; replace the direct Qt calls from the signal path with an
async-signal-safe notification mechanism: set a sig_atomic_t flag or write a
byte to a self-pipe in signalHandler2, and change shutdown_handler (or a new
poll/slot in main/Qt event loop) to perform the actual Qt work
(QTimer::singleShot, emit controller->reload) when the main loop notices the
flag or reads the pipe; ensure signalHandler2, any SIGCONT handler, and related
handlers touch only async-signal-safe globals/functions.
---
Nitpick comments:
In `@applications/system-service/dbusservice.cpp`:
- Around line 278-283: The dependants list for each API (api.dependants) must
act like a set, so don’t append the same DBus service multiple times; update the
logic in the block that currently does api.dependants->append(message.service())
to first check whether message.service() is already present (e.g., using
contains() or std::find) and only append if it isn’t, ensuring api.dependants
remains unique per DBus service (this affects the code paths around
serviceOwnerChanged() and releaseAPI() which clear entries).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7de97e3-c2e9-4e39-ade2-88150a4612c6
📒 Files selected for processing (125)
.clang-format.github/actions/sync-repository/action.yml.github/actions/web/action.yaml.github/workflows/build.yml.github/workflows/publish.yml.github/workflows/web.yaml.gitignoreMakefileapplications/desktop-file-edit/edit.cppapplications/desktop-file-edit/edit.happlications/desktop-file-edit/main.cppapplications/desktop-file-install/main.cppapplications/desktop-file-validate/main.cppapplications/display-server/connection.cppapplications/display-server/connection.happlications/display-server/dbusinterface.cppapplications/display-server/dbusinterface.happlications/display-server/display-server.proapplications/display-server/evdevdevice.cppapplications/display-server/evdevdevice.happlications/display-server/evdevhandler.cppapplications/display-server/evdevhandler.happlications/display-server/guithread.cppapplications/display-server/guithread.happlications/display-server/main.cppapplications/display-server/surface.cppapplications/display-server/surface.happlications/display-server/surfacewidget.cppapplications/display-server/surfacewidget.happlications/fbinfo/main.cppapplications/gio/cat.happlications/gio/common.cppapplications/gio/common.happlications/gio/copy.happlications/gio/help.happlications/gio/launch.happlications/gio/main.cppapplications/gio/mkdir.happlications/gio/open.happlications/gio/remove.happlications/gio/rename.happlications/gio/version.happlications/inject_evdev/main.cppapplications/launcher/appitem.cppapplications/launcher/appitem.happlications/launcher/controller.cppapplications/launcher/controller.happlications/launcher/launcher.proapplications/launcher/main.cppapplications/launcher/mxcfb.happlications/launcher/notificationlist.happlications/launcher/oxide_stable.happlications/launcher/widgets/CalendarMenu.qmlapplications/launcher/widgets/NotificationsPopup.qmlapplications/launcher/wifinetworklist.happlications/lockscreen/controller.happlications/lockscreen/lockscreen.proapplications/lockscreen/main.cppapplications/notify-send/main.cppapplications/process-manager/controller.happlications/process-manager/erode_stable.happlications/process-manager/main.cppapplications/process-manager/process-manager.proapplications/process-manager/taskitem.happlications/process-manager/tasklist.happlications/screenshot-viewer/anxiety_stable.happlications/screenshot-viewer/controller.happlications/screenshot-viewer/main.cppapplications/screenshot-viewer/screenshot-viewer.proapplications/screenshot-viewer/screenshotlist.happlications/settings-manager/main.cppapplications/system-service/apibase.cppapplications/system-service/apibase.happlications/system-service/application.cppapplications/system-service/application.happlications/system-service/appsapi.cppapplications/system-service/appsapi.happlications/system-service/bss.cppapplications/system-service/bss.happlications/system-service/controller.happlications/system-service/dbusservice.cppapplications/system-service/dbusservice.happlications/system-service/eventlistener.cppapplications/system-service/eventlistener.happlications/system-service/generate_xml.shapplications/system-service/main.cppapplications/system-service/network.cppapplications/system-service/network.happlications/system-service/notification.cppapplications/system-service/notification.happlications/system-service/notificationapi.cppapplications/system-service/notificationapi.happlications/system-service/powerapi.cppapplications/system-service/powerapi.happlications/system-service/screenapi.cppapplications/system-service/screenapi.happlications/system-service/screenshot.cppapplications/system-service/screenshot.happlications/system-service/system-service.proapplications/system-service/systemapi.cppapplications/system-service/systemapi.happlications/system-service/tarnish_stable.happlications/system-service/wifiapi.cppapplications/system-service/wifiapi.happlications/system-service/wlan.cppapplications/system-service/wlan.happlications/system-service/wpa_supplicant.cppapplications/system-service/wpa_supplicant.happlications/task-switcher/appitem.cppapplications/task-switcher/appitem.happlications/task-switcher/controller.happlications/task-switcher/corrupt_stable.happlications/task-switcher/main.cppapplications/task-switcher/task-switcher.proapplications/update-desktop-database/main.cppapplications/xclip/main.cppapplications/xdg-desktop-icon/main.cppapplications/xdg-desktop-menu/main.cppapplications/xdg-icon-resource/main.cppapplications/xdg-open/main.cppapplications/xdg-settings/main.cppinterfaces/blight.xmlpackageqmake/common.priqmake/epaper.pri
💤 Files with no reviewable changes (6)
- applications/system-service/notificationapi.h
- applications/system-service/notification.h
- applications/system-service/network.cpp
- applications/system-service/network.h
- applications/system-service/notification.cpp
- applications/system-service/notificationapi.cpp
✅ Files skipped from review due to trivial changes (40)
- .clang-format
- .gitignore
- applications/desktop-file-edit/edit.h
- applications/process-manager/process-manager.pro
- applications/system-service/eventlistener.h
- applications/process-manager/erode_stable.h
- applications/launcher/mxcfb.h
- applications/screenshot-viewer/anxiety_stable.h
- applications/system-service/apibase.h
- applications/display-server/surfacewidget.h
- applications/process-manager/controller.h
- applications/display-server/evdevhandler.h
- applications/gio/main.cpp
- applications/launcher/widgets/NotificationsPopup.qml
- applications/system-service/eventlistener.cpp
- applications/system-service/application.h
- applications/desktop-file-edit/main.cpp
- applications/gio/open.h
- applications/launcher/oxide_stable.h
- applications/gio/version.h
- applications/screenshot-viewer/controller.h
- applications/lockscreen/main.cpp
- applications/gio/mkdir.h
- applications/gio/launch.h
- applications/gio/help.h
- applications/desktop-file-validate/main.cpp
- applications/screenshot-viewer/screenshotlist.h
- applications/system-service/controller.h
- applications/launcher/notificationlist.h
- applications/gio/common.cpp
- applications/process-manager/tasklist.h
- applications/gio/rename.h
- applications/gio/copy.h
- applications/fbinfo/main.cpp
- applications/process-manager/taskitem.h
- applications/inject_evdev/main.cpp
- applications/gio/remove.h
- applications/display-server/main.cpp
- applications/desktop-file-edit/edit.cpp
- applications/launcher/appitem.cpp
| auto json = toJson(reg, QJsonDocument::Indented); | ||
| if (!toFile.open(QFile::WriteOnly | QFile::Truncate)) { | ||
| qDebug() << "Error on file" << path << ": Cannot write to file"; | ||
| success = false; | ||
| continue; | ||
| } | ||
| toFile.write(json.toUtf8()); | ||
| toFile.close(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around the reported lines
sed -n '80,160p' applications/desktop-file-install/main.cpp | nl -baRepository: Eeems-Org/oxide
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all occurrences of toFile.write / flush / close in that file
rg -n "toFile\.(write|flush|close|remove)" applications/desktop-file-install/main.cpp || trueRepository: Eeems-Org/oxide
Length of output: 128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show declarations/usages of QFile toFile
rg -n "QFile\s+toFile|toFile\s*\(" applications/desktop-file-install/main.cpp || trueRepository: Eeems-Org/oxide
Length of output: 84
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print a larger window around the reported lines with line numbers (cat -n)
sed -n '60,150p' applications/desktop-file-install/main.cpp | cat -nRepository: Eeems-Org/oxide
Length of output: 2652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for any flush handling / error handling related to toFile
rg -n "toFile\.(write|flush|close|open|error)|QFile::errorString" applications/desktop-file-install/main.cpp || trueRepository: Eeems-Org/oxide
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the declaration block for toFile (around the match)
rg -n "QFile toFile\(" -n applications/desktop-file-install/main.cpp || true
sed -n '70,100p' applications/desktop-file-install/main.cpp | cat -nRepository: Eeems-Org/oxide
Length of output: 1294
Check QFile::write()/flush() results to avoid partial .oxide output
After toFile.open(... | QFile::Truncate) the code ignores possible short-write/flush failures (toFile.write(...) result and any flush() result), which can leave a truncated/partial destination file while the iteration still looks successful.
Suggested fix
- auto json = toJson(reg, QJsonDocument::Indented);
+ auto json = toJson(reg, QJsonDocument::Indented).toUtf8();
if (!toFile.open(QFile::WriteOnly | QFile::Truncate)) {
qDebug() << "Error on file" << path << ": Cannot write to file";
success = false;
continue;
}
- toFile.write(json.toUtf8());
+ if (toFile.write(json) != json.size() || !toFile.flush()) {
+ qDebug() << "Error on file" << path << ": Failed to write file";
+ toFile.close();
+ toFile.remove();
+ success = false;
+ continue;
+ }
toFile.close();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto json = toJson(reg, QJsonDocument::Indented); | |
| if (!toFile.open(QFile::WriteOnly | QFile::Truncate)) { | |
| qDebug() << "Error on file" << path << ": Cannot write to file"; | |
| success = false; | |
| continue; | |
| } | |
| toFile.write(json.toUtf8()); | |
| toFile.close(); | |
| auto json = toJson(reg, QJsonDocument::Indented).toUtf8(); | |
| if (!toFile.open(QFile::WriteOnly | QFile::Truncate)) { | |
| qDebug() << "Error on file" << path << ": Cannot write to file"; | |
| success = false; | |
| continue; | |
| } | |
| if (toFile.write(json) != json.size() || !toFile.flush()) { | |
| qDebug() << "Error on file" << path << ": Failed to write file"; | |
| toFile.close(); | |
| toFile.remove(); | |
| success = false; | |
| continue; | |
| } | |
| toFile.close(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@applications/desktop-file-install/main.cpp` around lines 113 - 120, Check the
result of toFile.write(...) and toFile.flush() after opening and before closing
the file: call toFile.write() and verify the returned qint64 equals the JSON
byte length (json.toUtf8().size()), then call toFile.flush() and ensure it
returns true; on any mismatch or failure, log an error including path, set
success = false and continue, then close the file. Locate the block around the
variables json, toFile, path and the calls to toFile.open(), toFile.write(), and
toFile.close() and add these checks and error handling for short-writes/flush
failures.
| int fds[2]; | ||
| if(::socketpair(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, fds) == -1){ | ||
| if (::socketpair(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, fds) == -1) { | ||
| C_WARNING("Unable to open socket pair:" << strerror(errno)); | ||
| fds[0] = fds[1] = -1; | ||
| } | ||
| m_clientFd = fds[0]; | ||
| m_serverFd = fds[1]; | ||
| m_notifier = new QSocketNotifier(m_serverFd, QSocketNotifier::Read, this); | ||
| connect(m_notifier, &QSocketNotifier::activated, this, &Connection::readSocket); | ||
| connect( | ||
| m_notifier, &QSocketNotifier::activated, this, &Connection::readSocket | ||
| ); | ||
|
|
||
| if(::socketpair(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, fds) == -1){ | ||
| if (::socketpair(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, fds) == -1) { | ||
| C_WARNING("Unable to open input socket pair:" << strerror(errno)); | ||
| fds[0] = fds[1] = -1; | ||
| } | ||
| m_clientInputFd = fds[0]; | ||
| m_serverInputFd = fds[1]; |
There was a problem hiding this comment.
Abort connection setup when either socket pair cannot be created.
After either socketpair() failure, the constructor still stores -1 descriptors and keeps going. The rest of the object then reads from or writes to invalid fds instead of failing cleanly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@applications/display-server/connection.cpp` around lines 63 - 80, Connection
constructor must abort when either socketpair() fails: detect failure of the
first or second ::socketpair call and do not continue initializing with -1 fds;
close any fds that were successfully created (m_clientFd/m_serverFd or
m_clientInputFd/m_serverInputFd) and clean up m_notifier if already allocated,
then throw an exception (e.g., std::runtime_error) to abort construction.
Specifically, after the first ::socketpair call, if it fails, ensure you close
any non -1 entries in fds and throw; after creating m_notifier, if the second
::socketpair fails, close the previously created m_clientFd/m_serverFd and any
fds from the failed call before throwing; alternatively introduce a bool valid
flag set only after both socketpairs succeed and only construct m_notifier after
both succeed—use function names/fields socketpair, m_clientFd, m_serverFd,
m_clientInputFd, m_serverInputFd, m_notifier, and Connection::readSocket to
locate and implement the fix.
There was a problem hiding this comment.
I don't see the fds being used later in the constructor.
There was a problem hiding this comment.
@Eeems, you're right that the local fds[] array itself isn't used again after the assignments. The concern I should have been clearer about is what happens to the values assigned from fds immediately afterwards.
Specifically, after the first socketpair() failure sets m_serverFd = -1, the very next statement passes it directly to QSocketNotifier:
m_clientFd = fds[0]; // -1 on failure
m_serverFd = fds[1]; // -1 on failure
m_notifier = new QSocketNotifier(m_serverFd, QSocketNotifier::Read, this); // QSocketNotifier(-1, ...)QSocketNotifier created with fd -1 won't work correctly — Qt will emit a warning and the notifier will never fire, leaving the connection silently broken with no way for the caller to detect it at construction time (unless they explicitly call isValid() afterwards).
The rest is handled gracefully: isValid() checks m_clientFd > 0 && m_serverFd > 0, and ::close(-1) in the destructor just returns EBADF harmlessly.
Would you like to open a follow-up issue to track guarding the QSocketNotifier construction (or returning early before it) when socketpair() fails?
| void remove(const QDBusObjectPath& path) | ||
| { | ||
| QMutableListIterator<WifiNetwork*> i(networks); | ||
| while(i.hasNext()){ | ||
| while (i.hasNext()) { | ||
| auto network = i.next(); | ||
| network->remove(path); | ||
| if(!network->paths().length()){ | ||
| if (!network->paths().length()) { | ||
| qDebug() << "Network removed" << network->ssid(); | ||
| i.remove(); | ||
| delete network; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target code region
sed -n '300,420p' applications/launcher/wifinetworklist.h
# Find the class definition to understand what kind of model it is
rg -n "class\s+|QAbstractListModel|beginRemoveRows|endRemoveRows|remove\s*\(\s*const\s+QDBusObjectPath&\s*path\s*\)" applications/launcher/wifinetworklist.h
# Search the repository for beginRemoveRows/endRemoveRows usage in related files
rg -n "beginRemoveRows|endRemoveRows" applications/launcher | head -n 200
# Find other remove/row removal methods in this file (or neighbors)
rg -n "void\s+remove\s*\(|remove\s*\(" applications/launcher/wifinetworklist.hRepository: Eeems-Org/oxide
Length of output: 5137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the full WifiNetworkList model methods around remove and rowCount/data
sed -n '120,420p' applications/launcher/wifinetworklist.hRepository: Eeems-Org/oxide
Length of output: 8324
Wrap row removals with model notifications in WifiNetworkList::remove(const QDBusObjectPath& path).
remove(path) removes from networks (and deletes WifiNetwork) without beginRemoveRows/endRemoveRows, breaking QAbstractListModel update contracts; other removals in this model (remove(QString), removeUnknown(), clear()) already do the proper notifications. Add beginRemoveRows/endRemoveRows around each removed row (using the index before i.remove()), or remove consecutive ranges.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@applications/launcher/wifinetworklist.h` around lines 345 - 356, The removal
loop in WifiNetworkList::remove(const QDBusObjectPath& path) deletes WifiNetwork
instances from the networks list without emitting model change notifications;
wrap each removal with QAbstractListModel notifications by calling
beginRemoveRows(parent, row, row) immediately before removing the element (use
the current index of the iterator/network in networks) and endRemoveRows()
immediately after i.remove()/delete network, or detect and remove consecutive
ranges and call beginRemoveRows/endRemoveRows once per range so the model
contract is preserved.
| bool scanning() { return api->scanning(); } | ||
| Q_INVOKABLE void scan(bool active) { api->scan(active).waitForFinished(); } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around the reported lines
sed -n '330,430p' applications/launcher/wifinetworklist.h | cat -n
# Find where `api` is declared and `setAPI()` is defined/used
rg -n "setAPI\\b|\\bapi\\b|scanning\\(\\)|scan\\(bool" applications/launcher/wifinetworklist.h
# Search for other definitions/usages of scanning()/scan() and where QML calls might happen
rg -n "scanning\\(\\)|scan\\(bool" applications/launcher -S
# If present, find the type of `api` and how it's set (constructor defaults, nullptr usage, etc.)
rg -n "class|struct|Q_PROPERTY|Q_INVOKABLE|signals?:|slots?:|private:" applications/launcher/wifinetworklist.hRepository: Eeems-Org/oxide
Length of output: 4776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the WifiNetworkList class definition including constructor/initialization and scanning property
sed -n '120,420p' applications/launcher/wifinetworklist.h | cat -n
# Find all call sites of setAPI() for WifiNetworkList
rg -n "setAPI\\(" applications/launcher -S
# Find where WifiNetworkList is created/owned (constructors/usages)
rg -n "new\\s+WifiNetworkList\\b|WifiNetworkList\\s*\\*|WifiNetworkList\\b" applications/launcher -SRepository: Eeems-Org/oxide
Length of output: 11304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,220p' applications/launcher/controller.h | cat -n
sed -n '220,310p' applications/launcher/controller.h | cat -n
sed -n '480,560p' applications/launcher/controller.h | cat -n
sed -n '880,980p' applications/launcher/controller.h | cat -nRepository: Eeems-Org/oxide
Length of output: 13710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "networks\\.?scanning\\b|scanningChanged\\b|\\bscan\\s*\\(" -S . --glob='*.qml' --glob='*.js' --glob='*.ts'
# Also check for direct invocations by exposing the model under some name
rg -n "\\bscanning\\b" applications -S --glob='*.qml' --glob='*.ts' --glob='*.h' --glob='*.cpp'
# Find where WifiNetworkList is exposed to QML (property name)
rg -n "getNetworks\\b|Q_PROPERTY\\(.*networks|NOTIFY.*networksChanged" applications/launcher/controller.h -nRepository: Eeems-Org/oxide
Length of output: 1683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' applications/launcher/widgets/WifiMenu.qml | cat -nRepository: Eeems-Org/oxide
Length of output: 7228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where WifiMenu is instantiated
rg -n "WifiMenu\\b" applications/launcher -S --glob='*.qml'
# Inspect the top-level QML files around where the popup visibility is toggled
# (narrow to likely candidates by searching for "wifi" menus / opening/visible bindings)
rg -n "open|visible\\s*:\\s*|onVisibleChanged\\s*:\\s*|wifiMenu|WifiMenu" applications/launcher -S --glob='*.qml'Repository: Eeems-Org/oxide
Length of output: 2052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '340,460p' applications/launcher/main.qml | cat -nRepository: Eeems-Org/oxide
Length of output: 5943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find launcher entrypoint / where Controller is instantiated
rg -n "new\\s+Controller\\b|Controller\\s*\\*" applications/launcher -S --glob='*.cpp' --glob='*.h'
# Find where Controller is set as a context property / registered to QML
rg -n "setContextProperty\\(|rootContext\\b|QQmlContext|engine\\b|QQmlApplicationEngine|load\\(" applications/launcher -S --glob='*.cpp'
# Show the relevant launcher main file(s)
fd -e cpp applications/launcher | head -n 50Repository: Eeems-Org/oxide
Length of output: 1442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' applications/launcher/main.cpp | cat -nRepository: Eeems-Org/oxide
Length of output: 4014
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "new\\s+WifiNetworkList\\b|WifiNetworkList\\s*\\(" applications/launcher -S
rg -n "new\\s+WifiNetworkList\\b|WifiNetworkList\\s*\\(" -S .Repository: Eeems-Org/oxide
Length of output: 465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "networks->setAPI\\b|\\.setAPI\\(wifiApi\\b|setAPI\\(" applications/launcher -S
rg -n "\\bapi\\s*=\\s*nullptr\\b" applications/launcher/wifinetworklist.h applications/launcher -SRepository: Eeems-Org/oxide
Length of output: 512
Guard null api in WifiNetworkList QML-exposed scanning methods.
applications/launcher/wifinetworklist.h has WifiNetworkList::api defaulting to nullptr, but scanning() and scan() dereference it unconditionally. Current launcher code sets networks->setAPI(wifiApi) before loading QML, so this is likely not hit on initial UI—but it’s still a fragile footgun if these methods are called before setAPI().
Proposed fix
- bool scanning() { return api->scanning(); }
- Q_INVOKABLE void scan(bool active) { api->scan(active).waitForFinished(); }
+ bool scanning() { return api != nullptr && api->scanning(); }
+ Q_INVOKABLE void scan(bool active)
+ {
+ if (api != nullptr) {
+ api->scan(active).waitForFinished();
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool scanning() { return api->scanning(); } | |
| Q_INVOKABLE void scan(bool active) { api->scan(active).waitForFinished(); } | |
| bool scanning() { return api != nullptr && api->scanning(); } | |
| Q_INVOKABLE void scan(bool active) | |
| { | |
| if (api != nullptr) { | |
| api->scan(active).waitForFinished(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@applications/launcher/wifinetworklist.h` around lines 378 - 379,
WifiNetworkList's QML-exposed methods dereference the possibly-null member api
(methods scanning() and scan(bool)), so add null guards: in bool
WifiNetworkList::scanning() return false if api is nullptr, and in Q_INVOKABLE
void WifiNetworkList::scan(bool active) early-return (no-op) if api is nullptr
instead of calling api->scan(active).waitForFinished();; reference the
WifiNetworkList class and its api member and update scanning() and scan(bool) to
check api before use (optionally log or warn when api is null).
| if (image == nullptr) { | ||
| file = new QFile(); | ||
| auto compositor = getCompositorDBus(); | ||
| auto reply = compositor->frameBuffer(); | ||
| reply.waitForFinished(); | ||
| if(reply.isError()){ | ||
| O_WARNING("Failed to get framebuffer fd" << reply.error().message()); | ||
| if (reply.isError()) { | ||
| O_WARNING( | ||
| "Failed to get framebuffer fd" << reply.error().message() | ||
| ); | ||
| return nullptr; | ||
| } | ||
| QDBusUnixFileDescriptor qfd = reply.value(); | ||
| if(!qfd.isValid()){ | ||
| if (!qfd.isValid()) { | ||
| O_WARNING("Framebuffer fd is not valid"); | ||
| return nullptr; | ||
| } | ||
| int fd = dup(qfd.fileDescriptor()); | ||
| if(fd < 0){ | ||
| if (fd < 0) { | ||
| O_WARNING("Failed to get framebuffer fd" << std::strerror(errno)); | ||
| return nullptr; | ||
| } | ||
| if(!file->open(fd, QFile::ReadWrite)){ | ||
| O_WARNING("Failed to open framebuffer" << file->errorString()); | ||
| O_DEBUG("Framebuffer fd:" << std::to_string(fd).c_str()) | ||
| if (!file->open(fd, QFile::ReadWrite)) { | ||
| O_WARNING("Failed to open framebuffer " << file->errorString()); | ||
| ::close(fd); | ||
| delete file; | ||
| file = nullptr; | ||
| return nullptr; | ||
| } | ||
| auto stride = deviceSettings.getDeviceType() == Oxide::DeviceSettings::RM1 ? 2816 : 2808; | ||
| uchar* data = file->map(0, stride * 1872); | ||
| if(data == nullptr){ | ||
| int stride = deviceSettings.getScreenStride(); | ||
| uchar* data = file->map(0, stride * deviceSettings.getScreenHeight()); | ||
| if (data == nullptr) { | ||
| O_WARNING("Failed to map framebuffer" << file->errorString()); | ||
| file->close(); | ||
| delete file; | ||
| file = nullptr; | ||
| return nullptr; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# locate file
ls -la applications/system-service/apibase.cpp
# show surrounding code with line numbers
sed -n '1,140p' applications/system-service/apibase.cpp | nl -ba | sed -n '1,140p'Repository: Eeems-Org/oxide
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# extract around likely function boundaries for getFrameBuffer
rg -n "getFrameBuffer|getFramebuffer|frameBuffer" -S applications/system-service/apibase.cppRepository: Eeems-Org/oxide
Length of output: 337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# show more lines from snippet start to end (lines 40-140)
sed -n '35,140p' applications/system-service/apibase.cpp | nl -baRepository: Eeems-Org/oxide
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# show further lines beyond 140 to see success path cleanup
sed -n '140,260p' applications/system-service/apibase.cpp | nl -baRepository: Eeems-Org/oxide
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search other usages of QFile::open(fd, QFile::ReadWrite) with file handle flags in repo
rg -n "QFile::AutoCloseHandle|DontCloseHandle|open\(fd, QFile::ReadWrite" -S --glob='*.cpp' --glob='*.h' . | head -n 50Repository: Eeems-Org/oxide
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set -x
file="applications/system-service/apibase.cpp"
if [ ! -f "$file" ]; then echo "missing $file"; exit 1; fi
echo "=== file info ==="
wc -l "$file"
echo "=== rg matches for framebuffer ==="
rg -n "getFrameBuffer|getFramebuffer|frameBuffer" -S "$file" || true
echo "=== lines 35-170 ==="
sed -n '35,170p' "$file" | nl -ba
echo "=== lines 170-320 ==="
sed -n '170,320p' "$file" | nl -ba
echo "=== repo-wide QFile::open(fd, ...) patterns (limited) ==="
rg -n "QFile::AutoCloseHandle|DontCloseHandle|open\(\s*fd\s*,\s*QFile::ReadWrite" -S --glob='*.cpp' --glob='*.h' . | head -n 80 || trueRepository: Eeems-Org/oxide
Length of output: 931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set -x
file="applications/system-service/apibase.cpp"
wc -l "$file"
rg -n "getFrameBuffer|getFramebuffer|frameBuffer" -S "$file" || true
sed -n '35,170p' "$file" | nl -ba
sed -n '170,320p' "$file" | nl -ba
rg -n "QFile::AutoCloseHandle|DontCloseHandle|open\(\s*fd\s*,\s*QFile::ReadWrite" -S --glob='*.cpp' --glob='*.h' . | head -n 80 || trueRepository: Eeems-Org/oxide
Length of output: 698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set -x
file="applications/system-service/apibase.cpp"
wc -l "$file"
rg -n "getFrameBuffer|getFramebuffer|frameBuffer" -S "$file" || true
sed -n '35,170p' "$file" | nl -ba
sed -n '170,320p' "$file" | nl -ba
rg -n "QFile::AutoCloseHandle|DontCloseHandle|open\(\s*fd\s*,\s*QFile::ReadWrite" -S --glob='*.cpp' --glob='*.h' . | head -n 80 || trueRepository: Eeems-Org/oxide
Length of output: 698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="applications/system-service/apibase.cpp"
echo "=== wc ==="
wc -l "$file"
echo "=== rg framebuffer ==="
rg -n "getFrameBuffer|getFramebuffer|frameBuffer" -S "$file" || true
echo "=== sed 35-170 ==="
sed -n '35,170p' "$file" | nl -ba
echo "=== sed 170-320 ==="
sed -n '170,320p' "$file" | nl -ba
echo "=== repo-wide patterns ==="
rg -n "QFile::AutoCloseHandle|DontCloseHandle|open\(\s*fd\s*,\s*QFile::ReadWrite" -S --glob='*.cpp' --glob='*.h' . | head -n 80 || trueRepository: Eeems-Org/oxide
Length of output: 495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="applications/system-service/apibase.cpp"
echo "=== getFrameBuffer() implementation lines 40-130 ==="
sed -n '40,130p' "$file" | cat -n
echo "=== lines 130-190 (cleanup / use sites) ==="
sed -n '130,190p' "$file" | cat -n
echo "=== exact QFile::open call line ==="
rg -n "file->open\\(fd, QFile::ReadWrite\\)" "$file" || trueRepository: Eeems-Org/oxide
Length of output: 4869
🌐 Web query:
Qt QFile::open(int fd, OpenMode mode, FileHandleFlags flags=DontCloseHandle) AutoCloseHandle closes fd on failure
💡 Result:
No, passing the AutoCloseHandle flag to QFile::open(int fd,...) does not cause the file descriptor to be closed if the open operation fails [1][2][3]. According to the official Qt documentation, the AutoCloseHandle flag specifically controls the behavior of the QFile::close method after the file has been successfully opened [1][4]. If AutoCloseHandle is specified and the open call succeeds, the QFile object takes ownership of the file descriptor, and subsequent calls to close will close the descriptor [1][5]. If the open call fails, the QFile object does not successfully adopt the handle, and therefore the logic associated with AutoCloseHandle is not triggered [1][2]. In such cases, the caller remains responsible for managing the file descriptor [1].
Citations:
- 1: https://doc.qt.io/qt-6.5/qfile.html
- 2: https://doc.qt.io/qt-6.10/qfile.html
- 3: https://doc.qt.io/qt-6/qfile.html
- 4: https://www.copperspice.com/docs/cs_api/class_qfile.html
- 5: https://stuff.mit.edu/afs/athena/software/texmaker_v5.0.2/qt57/doc/qtcore/qfile.html
Fix APIBase::getFrameBuffer() fd/QFile ownership on error paths
file = new QFile()is allocated before DBus/dup()failures and is not deleted on early returns (reply.isError(),!qfd.isValid(),fd < 0), leaking the heap object (and re-allocating on each retry).- After
dup(),file->open(fd, QFile::ReadWrite)uses Qt’s defaultDontCloseHandle, so on later failures after a successfulopen()(map()failure / null/emptyQImage),file->close()+delete filewon’t close the duplicatedfd, leaking descriptors across retries.
Suggested fix
- file = new QFile();
auto compositor = getCompositorDBus();
auto reply = compositor->frameBuffer();
reply.waitForFinished();
if (reply.isError()) {
O_WARNING(
@@
int fd = dup(qfd.fileDescriptor());
if (fd < 0) {
O_WARNING("Failed to get framebuffer fd" << std::strerror(errno));
return nullptr;
}
+ file = new QFile();
O_DEBUG("Framebuffer fd:" << std::to_string(fd).c_str())
- if (!file->open(fd, QFile::ReadWrite)) {
+ if (!file->open(
+ fd, QFile::ReadWrite, QFileDevice::AutoCloseHandle
+ )) {
O_WARNING("Failed to open framebuffer " << file->errorString());
::close(fd);
delete file;
file = nullptr;
return nullptr;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (image == nullptr) { | |
| file = new QFile(); | |
| auto compositor = getCompositorDBus(); | |
| auto reply = compositor->frameBuffer(); | |
| reply.waitForFinished(); | |
| if(reply.isError()){ | |
| O_WARNING("Failed to get framebuffer fd" << reply.error().message()); | |
| if (reply.isError()) { | |
| O_WARNING( | |
| "Failed to get framebuffer fd" << reply.error().message() | |
| ); | |
| return nullptr; | |
| } | |
| QDBusUnixFileDescriptor qfd = reply.value(); | |
| if(!qfd.isValid()){ | |
| if (!qfd.isValid()) { | |
| O_WARNING("Framebuffer fd is not valid"); | |
| return nullptr; | |
| } | |
| int fd = dup(qfd.fileDescriptor()); | |
| if(fd < 0){ | |
| if (fd < 0) { | |
| O_WARNING("Failed to get framebuffer fd" << std::strerror(errno)); | |
| return nullptr; | |
| } | |
| if(!file->open(fd, QFile::ReadWrite)){ | |
| O_WARNING("Failed to open framebuffer" << file->errorString()); | |
| O_DEBUG("Framebuffer fd:" << std::to_string(fd).c_str()) | |
| if (!file->open(fd, QFile::ReadWrite)) { | |
| O_WARNING("Failed to open framebuffer " << file->errorString()); | |
| ::close(fd); | |
| delete file; | |
| file = nullptr; | |
| return nullptr; | |
| } | |
| auto stride = deviceSettings.getDeviceType() == Oxide::DeviceSettings::RM1 ? 2816 : 2808; | |
| uchar* data = file->map(0, stride * 1872); | |
| if(data == nullptr){ | |
| int stride = deviceSettings.getScreenStride(); | |
| uchar* data = file->map(0, stride * deviceSettings.getScreenHeight()); | |
| if (data == nullptr) { | |
| O_WARNING("Failed to map framebuffer" << file->errorString()); | |
| file->close(); | |
| delete file; | |
| file = nullptr; | |
| return nullptr; | |
| if (image == nullptr) { | |
| auto compositor = getCompositorDBus(); | |
| auto reply = compositor->frameBuffer(); | |
| reply.waitForFinished(); | |
| if (reply.isError()) { | |
| O_WARNING( | |
| "Failed to get framebuffer fd" << reply.error().message() | |
| ); | |
| return nullptr; | |
| } | |
| QDBusUnixFileDescriptor qfd = reply.value(); | |
| if (!qfd.isValid()) { | |
| O_WARNING("Framebuffer fd is not valid"); | |
| return nullptr; | |
| } | |
| int fd = dup(qfd.fileDescriptor()); | |
| if (fd < 0) { | |
| O_WARNING("Failed to get framebuffer fd" << std::strerror(errno)); | |
| return nullptr; | |
| } | |
| file = new QFile(); | |
| O_DEBUG("Framebuffer fd:" << std::to_string(fd).c_str()) | |
| if (!file->open( | |
| fd, QFile::ReadWrite, QFileDevice::AutoCloseHandle | |
| )) { | |
| O_WARNING("Failed to open framebuffer " << file->errorString()); | |
| ::close(fd); | |
| delete file; | |
| file = nullptr; | |
| return nullptr; | |
| } | |
| int stride = deviceSettings.getScreenStride(); | |
| uchar* data = file->map(0, stride * deviceSettings.getScreenHeight()); | |
| if (data == nullptr) { | |
| O_WARNING("Failed to map framebuffer" << file->errorString()); | |
| file->close(); | |
| delete file; | |
| file = nullptr; | |
| return nullptr; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@applications/system-service/apibase.cpp` around lines 55 - 91,
APIBase::getFrameBuffer() currently allocates file = new QFile() before
DBus/dup() and uses QFile::open(fd, QFile::ReadWrite) which by default keeps the
duplicated fd open on QFile deletion, causing leaked QFile objects on early
returns and leaked fds after open failures; fix by deferring construction of the
QFile until after you have a valid duplicated fd (after
dup(qfd.fileDescriptor()) succeeds) or ensure every early-return path deletes
the previously-allocated QFile, and when opening with file->open(fd, ...) set
the QFile handle ownership flag so the QFile will close the underlying fd on
close/delete (i.e. use the QFile open variant/flag that makes QFile take
ownership), and also ensure that if file->map() or subsequent QImage
construction fails you explicitly close the fd (or rely on QFile ownership)
before deleting file to avoid descriptor leaks.
| if (!m_process->setUser(user()) || | ||
| m_process->setGroup(group())) { | ||
| O_WARNING(("Failed to set user/group for the process")); |
There was a problem hiding this comment.
Invert the setGroup() check and fail closed.
m_process->setGroup(group()) is currently un-negated, so success logs a warning while a failed group lookup is silently ignored. In the failure case the child can still start with the default gid.
Suggested fix
- if (!m_process->setUser(user()) ||
- m_process->setGroup(group())) {
+ if (!m_process->setUser(user()) ||
+ !m_process->setGroup(group())) {
O_WARNING(("Failed to set user/group for the process"));
+ return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@applications/system-service/application.cpp` around lines 91 - 93, The
conditional incorrectly negates only setUser and not setGroup causing successful
setGroup to trigger the warning; change the check so it fails when either
m_process->setUser(user()) or m_process->setGroup(group()) returns false (i.e.,
if (!m_process->setUser(user()) || !m_process->setGroup(group())) ), so the
O_WARNING(("Failed to set user/group for the process")) is executed on any
failure and the child is prevented from continuing; update the condition around
m_process->setUser, m_process->setGroup, user(), group() and ensure the failure
branch still handles cleanup/abort as intended.
2 participants
The pen input device is flipped, and refreshes are slow, but other than that, it works :)
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Build / CI