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

Change model query in place for Journal ListView, fixes #4852 #542

Closed

Conversation

samdroid-apps
Copy link
Contributor

This commit changes the way the list view model updates occur.
Instead of replacing the ListModel, we now just change the result set
query.

This means that the buggy scroll position setting code is no longer
needed. Infact, a lot of code is removed

Steps to test:

  • Go to the Journal
  • Open an item in the details view
  • Change title or star status
  • Click back
  • Notice how it has updated and the scroll position preserved

Other things tested:

  • Changing mountpoints
  • Selections and changing mountpoints
  • Selection preservation after refresh
  • Changing the filter

This commit changes the way the list view model updates occur.
Instead of replacing the ListModel, we now just change the result set
query.

This means that the buggy scroll position setting code is no longer
needed.  Infact, a lot of code is removed

Steps to test:

* Go to the Journal
* Open an item in the details view
* Change title or star status
* Click back
* Notice how it has updated and the scroll position preserved

Other things tested:

* Changing mountpoints
* Selections and changing mountpoints
* Selection preservation after refresh
* Changing the filter
@samdroid-apps
Copy link
Contributor Author

Ok, I will add the feature tag because this does do a lot of refactoring.

@godiard, @tchx84 Could this get merged for 106? If not, maybe we could merge the more self-contained #520 and target this for 108? Or will we just ship 106 with the bug?

@samdroid-apps
Copy link
Contributor Author

Ok, let's aim to 108 for this patch!

@quozl
Copy link
Contributor

quozl commented Jul 4, 2015

Is there a feature page? I don't understand your pull request yet; it doesn't look like a feature, but rather an optimisation. But I don't understand the impact. Is there a bug number that would tell me more?

@samdroid-apps
Copy link
Contributor Author

Yes it is an optimization, but it also fixes http://bugs.sugarlabs.org/ticket/4852

@quozl
Copy link
Contributor

quozl commented Jul 4, 2015

Thanks, I've reproduced 4852 now.

@tchx84
Copy link
Member

tchx84 commented Jul 22, 2015

Is this a replacement for #520?

@godiard
Copy link
Contributor

godiard commented Jul 22, 2015

@tchx84, no, is a complement. This patch solves the case of going to the detail view, modify the metadata (change star, title or description) and go back to the listview

@@ -92,8 +90,19 @@ def __init__(self, query):
# avoid hitting D-Bus and disk.
self.view_is_resizing = False

def set_query(self, query, clear_selection=True):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we were merging this along side #520, we would need to reset the self._updated_entries table here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @samdroid-apps I am slow with reviews this week. Could you prepare a new pr with the patch in #520, and this one modified to make easier test all together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do, see #564

@samdroid-apps
Copy link
Contributor Author

Replaced with #564

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

Successfully merging this pull request may close these issues.

4 participants