Skip to content

Conversation

ben-grande
Copy link
Contributor

@ben-grande ben-grande commented May 22, 2025

@ben-grande ben-grande force-pushed the preload-dispvm branch 6 times, most recently from 8a9c8b1 to 9369545 Compare May 22, 2025 14:38
@ben-grande ben-grande changed the title [WIP] Preload dispvm Add disposable preload setting to advanced settings May 22, 2025
@ben-grande ben-grande force-pushed the preload-dispvm branch 2 times, most recently from c639372 to 315926b Compare May 22, 2025 16:07
Copy link
Member

@marmarta marmarta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One black thing - while I do not love it, apparently we are to be using it, so could you put in an option to revert to default 88 characters per line? Marek approved it, and it really cuts down on black being absolutely ridiculous. I do it in this PR: https://github.com/QubesOS/qubes-desktop-linux-manager/pull/258/files#diff-037ea159eb0a7cb0ac23b851e66bee30fb838ee8d0d99fa331a1ba65283d37f7

vm_memory = getattr(self.vm, "memory", None)
vm_maxmem = getattr(self.vm, "maxmem", None)
vm_preload_dispvm = int(
self.vm.features.get("preload-dispvm-max", 0) or 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little bit confused about this, why is there or 0 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The , 0 is for when there is no value, the or 0 is for when the value is empty ''. I need to use the , 0 to not fail the .get() if there is no value to retrieve.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked again, the get(..., 0) is not necessary if there is or 0. It also doesn't fail the get() method without it. The or 0 covers more cases.

<x>0</x>
<y>0</y>
<width>836</width>
<width>858</width>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is accidental?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can revert this and other settings.

<locale language="English" country="UnitedStates"/>
</property>
<property name="currentIndex">
<number>0</number>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also accidental?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not on purpose, see comment below.

<widget class="QLabel" name="type_label">
<property name="font">
<font>
<weight>50</weight>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this and other font changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not on purpose, changed automatically by Qt Designer 6.4.2 on Debian, maybe I need to switch to Fedora to get the 6.9.0... https://packages.fedoraproject.org/pkgs/qt6-qttools/qt6-designer/.

QtCore.QRegularExpression(
"[a-zA-Z0-9_-]*",
QtCore.QRegularExpression.PatternOption.CaseInsensitiveOption,
# fmt: off
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why turning off formatting in such an awkward place (the middle of function declaration)? I do not like black myself, but I don't think this is a good approach to deal with its... opinions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used black -l80, with -l88, this is possibly unnecessary..


vm_memory = getattr(self.vm, "memory", None)
vm_maxmem = getattr(self.vm, "maxmem", None)
vm_preload_dispvm = int(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the preload-disposables changes should all be in one commit (and the qrexec and black stuff in their own commits)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is just for easier review and reverting if some tests show regression. By the end, it will be squashed.

else:
vm_preload_dispvm = 0
self.preload_dispvm.setMinimum(0)
self.preload_dispvm.setMaximum(9999)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not seem like a sensible maximum... (Also, max and min can be set in the ui file; easiest way to edit it is with designer-qt)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also set on the UI. I don't know a sensible maximum for this, the user can go crazy but the system won't preload if there is not enough memory. Is 50 a reasonable amount? I don't want to discourage users with a lot of memory in the future to not use the UI and use the terminal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

50 sounds like a reasonable amount, yeah

<item row="6" column="0">
<widget class="QLabel" name="preload_dispvm_label">
<property name="text">
<string>Preload disposables:</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs an explaining tooltip that clarifies what this setting does, exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ben-grande
Copy link
Contributor Author

One black thing

I though Qubes project was using -l80.... I formatted all my PRs with it. At least qubes-core-* is using it. I will format things on the manager and linux-desktop-manager with -l88 them.

@ben-grande ben-grande force-pushed the preload-dispvm branch 3 times, most recently from 942f71a to ab68aa1 Compare May 23, 2025 17:10
@ben-grande
Copy link
Contributor Author

Now I used black -l88.

@ben-grande ben-grande force-pushed the preload-dispvm branch 5 times, most recently from 4087322 to 2680b07 Compare June 3, 2025 11:43
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

Attention: Patch coverage is 77.57576% with 37 lines in your changes missing coverage. Please review.

Project coverage is 69.19%. Comparing base (bc659eb) to head (16f7bd0).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
qubesmanager/settings.py 77.57% 37 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   69.08%   69.19%   +0.11%     
==========================================
  Files          17       17              
  Lines        3891     3909      +18     
==========================================
+ Hits         2688     2705      +17     
- Misses       1203     1204       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ben-grande ben-grande force-pushed the preload-dispvm branch 2 times, most recently from a555607 to 207745b Compare June 3, 2025 16:32
@ben-grande
Copy link
Contributor Author

PipelineRetryFailed

if is_linux and self.init_mem.value() * 10 < self.max_mem_size.value():
self.warn_too_much_mem_label.setVisible(True)

def check_disp_changes(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this function actually do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing, outdated, removed.

@ben-grande
Copy link
Contributor Author

Failing test is unrelated to this PR and related to qubesimgconverter.

@ben-grande
Copy link
Contributor Author

PipelineRetryFailed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants