Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort filter proxy implementation. #106

Closed
wants to merge 2 commits into from
Closed

Conversation

mfitzp
Copy link
Collaborator

@mfitzp mfitzp commented Nov 24, 2020

This adds QSortFilterProxy sorting proxy models to the drift tab as an example. I chose this as I could see something there that could explain the issues you were experiencing before.

See the comments on the source for details.

@@ -814,7 +835,7 @@ def show_all_columns(delta_view):
Helper function to reset columns in the delta_model
:param delta_view: view to reset
"""
model = delta_view.model()
model = delta_view.model().sourceModel()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When you have a proxy in place, you need to reach "through" the proxy to get to the actual model. An alternative is just to keep a reference to the model around ourselves (e.g. self.delta_model) -- but if that is replaced, we still need to hook into the proxy model for it to work.

When you have an value on an object e.g. self.delta_model and replace it by assigning to it self.delta_model = MyNewModel() that changes the object in self.delta_model but all existing links are still pointing at the old object.

@@ -237,6 +247,9 @@ def __init__(self, parent):
layout_main.addWidget(main_vsplitter_window)
self.setLayout(layout_main)

# Call reset to initialize the table models.
self.reset()

def reset(self):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you have a reset method like this, it's is usually good to re-use it for the setup during init (to avoid code duplication).

@@ -246,9 +259,17 @@ def reset(self):
self.axes_drift_cont_upper.figure.canvas.draw()
self.axes_drift_cont_lower.figure.canvas.draw()
self.axes_drift_single.figure.canvas.draw()
self.delta_view.setModel(DeltaTableModel())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These lines changed the model on the view, but do not change what is pointed to e.g. by self.dg_samples_table -- so you now have two models existing at the same time. This might have been throwing the errors due to mismatches between items.

The DeltaTableModel() call doesn't keep a reference to that model, I'm 50:50 on whether that means the Python part would be garbage collected. If that happened you'd see segfaults.

@@ -224,8 +228,14 @@ def __init__(self, parent):
self.roman_label_widget.hide()

# dg table (Roman method)
self.dg_samples_view.setModel(self.dg_avg_model)
Copy link
Collaborator Author

@mfitzp mfitzp Nov 24, 2020

Choose a reason for hiding this comment

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

I find the names of the views and the models very confusing (they don't really match up in my head)!

@jkennedy-usgs jkennedy-usgs deleted the cr-sortmodel branch December 15, 2020 17:05
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