Skip to content

Merge PTZListModel and PTZPresetModel - #330

Merged
glikely merged 16 commits into
mainfrom
qt-model-refactor
Aug 9, 2026
Merged

Merge PTZListModel and PTZPresetModel#330
glikely merged 16 commits into
mainfrom
qt-model-refactor

Conversation

@glikely

@glikely glikely commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Removes the split-model design to clean up the separation between backend drivers and frontend UI

@EdueskaWeiz

Copy link
Copy Markdown
Contributor

The Windows job is red on a single narrowing conversion that -Werror promotes:

src/ptz-device.cpp(523,34): warning C4267: 'return': conversion from 'size_t' to 'int', possible loss of data
error C2220: the following warning is treated as an error

m_presetsDisplayOrder is a QList<size_t>, so presetAtDisplayRow() returns a size_t out of an int function. The function already uses -1 as its not-found sentinel, so an explicit cast preserves the current contract:

 int PTZDevice::presetAtDisplayRow(int row) const
 {
 	if (row < 0 || row >= presetCount())
 		return -1;
-	return m_presetsDisplayOrder[row];
+	return static_cast<int>(m_presetsDisplayOrder[row]);
 }

The other four jobs are already green, so that should be all that is left.

One heads-up for merge order: this PR moves the preset data into PTZDevice, which overlaps with the preset_max clamp in #323. Whichever lands second will need a rebase.

glikely added 15 commits August 8, 2026 15:59
To remove the need for UI elements to access PTZDevice instances
directly, pipe all of the change notifications through the dataChanged()
signal.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
This patch adds methods to the PTZListModel for reading/modifying the
device settings and fetching the properties. This way the UI views don't
need direct access to the PTZDevice instance.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
This moves state data out of the PTZPresetModel so that all
configuration is stored in the PTZDevice. In a follow on patch The
PTZPresetModel behaviour will be merged into PTZListModel. This is a
preparatory step.

On its own this isn't a great change. The moved data items are made
public in PTZDevice and the PTZPresetModel still accesses them directly.
This is to keep this change simple. After all the code is moved over
then the elements will be made protected again.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Restructuring code, no functional changes

Moves the preset save and restore code into the PTZDevice object itself
and removes it from the PTZPresetModel.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Refactor only, no functional change

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Refactor only, no functional change

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Refactor only, no functional change

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Refactor only, no functional change

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Refactor only, no functional change

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Refactor only, no functional change

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Replaces direct access to the data and uses a method call to create new
presets instead.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
@glikely
glikely force-pushed the qt-model-refactor branch from e30938e to a7900d7 Compare August 8, 2026 15:05
@EdueskaWeiz

Copy link
Copy Markdown
Contributor

Re-read the series against current main now that it is rebased. Two things.

Thanks for taking the narrowing fix — (int)m_presetsDisplayOrder[row] in presetAtDisplayRow() is what the Windows build was tripping over.

PTZListModel::insertRows() guards the wrong quantity

if (row < 0 || count <= 0 || row > ptz->presetCount() || row + count >= (int)ptz->maxPresets())

row is an insertion position, not a count, and the comparison needs to be against the resulting size. Two consequences:

One preset goes missing. on_actionPresetAdd_triggered() always appends, so row == presetCount(). With preset_max = 16 and 15 presets already there, row + count == 16 >= 16 and the "+" button silently stops working — 16 presets are configured, 15 are reachable. PTZPresetListModel::insertRows() allowed exactly 16 before this series, so this is a behaviour change inside a refactor.

A latent model/view desync. Inserting anywhere other than at the end while the list is full passes the guard: insertRows(0, 1, …) with 16 presets gives 0 + 1 >= 16 → false. beginInsertRows() is emitted, newPreset() then returns -1 because every id is taken, m_presetsDisplayOrder does not grow, and endInsertRows() announces a row the model does not have. Not reachable today since the only caller appends, but the old code structurally could not get into this state — it reserved the ids before beginInsertRows().

Suggested:

if (row < 0 || count <= 0 || row > ptz->presetCount() ||
    ptz->presetCount() + count > (int)ptz->maxPresets())
	return false;

and checking newPreset()'s return value inside the loop, to close the second one for good.

Small note in passing

The old PTZPresetListModel::insertRows() called beginInsertRows(parent, row, count), where the third argument should be the last row, row + count - 1. Your version has it right. That is a genuine fix rather than a move, so it may be worth a line in the commit message.

I have not run this build yet — the findings are from reading the diff. If it would help I can build the branch on Windows and exercise the preset list against OBS 32.2.1, same setup I used for #324.

@glikely

glikely commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

PTZListModel::insertRows() guards the wrong quantity

if (row < 0 || count <= 0 || row > ptz->presetCount() || row + count >= (int)ptz->maxPresets())

row is an insertion position, not a count, and the comparison needs to be against the resulting size. Two consequences:

Good catch, thanks.

@glikely
glikely force-pushed the qt-model-refactor branch from a7900d7 to a9487eb Compare August 9, 2026 13:04
Merging the two models together simplifies the overall data model and
makes it trivial to switch the view displayed in the presets list.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Reviewed-by: Eddy Weiz <eddyweiz@gmail.com>
@glikely
glikely force-pushed the qt-model-refactor branch from a9487eb to ad2495a Compare August 9, 2026 19:04
@glikely
glikely merged commit 9384726 into main Aug 9, 2026
6 checks passed
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.

2 participants