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

Starring/unstarring #50

Merged
merged 3 commits into from
Oct 25, 2018
Merged

Starring/unstarring #50

merged 3 commits into from
Oct 25, 2018

Conversation

redshiftzero
Copy link
Contributor

fix #17

needs merge of freedomofpress/securedrop-sdk#26 (discovered while working on this)

trying to star/unstar while not logged in

starring-not-logged-in

successful starring or unstarring

starring

(the first iteration as described in the ticket being not changing the ordering of sources in the list upon starring)

two things:

  1. I feel like the logic in 8a8e2ba should not be necessary, but otherwise the QSvgWidget did not seem to be repainting when the star icon changed 🤔(i'm missing something here)
  2. we need more robust logic for pushing changes to the server. i'm going to open a followup for this because this is a generic problem across a few issues that we should have a consistent strategy for

@ntoll
Copy link
Contributor

ntoll commented Oct 22, 2018

@redshiftzero hmmm... I'm having problems getting this to run and have had to modify the code a little (and I'm still getting errors).

I've had to add:

self.main_view.setup(controller)

As line 73 of gui/main.py, since the app wasn't starting. The app just crashed and I'll investigate further. I wonder what's different between your setup and mine. Hmm... I suspect my securedrop-sdk may be out of date perhaps? (I'll check further and report progress here).

@ntoll
Copy link
Contributor

ntoll commented Oct 22, 2018

OK... so the crash was down to clicking the star but not in such a way that it was possible to select the item to which it belonged, so a None was passed to show_conversation_for. Ugh... that took some finding and I'll fix.

Still getting "fail" messages when I try to star/unstar. I'll look at that next.

@redshiftzero
Copy link
Contributor Author

Unfortunately you do need this SDK change for this to work: freedomofpress/securedrop-sdk#26

@ntoll
Copy link
Contributor

ntoll commented Oct 22, 2018

@redshiftzero yeah... I just followed the exception in the logs and came to the same conclusion. ;-)

@ntoll
Copy link
Contributor

ntoll commented Oct 23, 2018

This works (now I've applied the pending patch to the SDK). LGTM.

redshiftzero and others added 3 commits October 24, 2018 21:32
The QSvgWidget doesn't seem to be updating on subsequent calls
of the QSvgWidget.load() method - this is a workaround
@redshiftzero
Copy link
Contributor Author

apologies @ntoll I missed your LGTMing this during the release fun - if you still agree this looks good (all I did was rebase to fix the conflicts), you should now be able to add an approving review and merge. I gave ya merge rights just now 😇

just a note: there is a TODO in the diff here which I started working on in a separate branch in c52d6a4 (not complete so not ready for review), which is modifying the sync logic here to square with the decisions in #51, but I think it makes sense to do that in a followup since the diff in this PR is a bit sprawling

@redshiftzero
Copy link
Contributor Author

ah i want to rebase and add a commit to #64, but want these changes in first to reduce wip diffs which conflict one another, so im gonna merge this and then rebase #64 on top of this and #65

@redshiftzero redshiftzero merged commit 456f248 into master Oct 25, 2018
@redshiftzero redshiftzero deleted the starring-unstarring branch October 25, 2018 18:58
@ntoll
Copy link
Contributor

ntoll commented Oct 29, 2018

Heh... feels like a game of PR bingo. Glad to see this all resolving.

legoktm pushed a commit that referenced this pull request Dec 11, 2023
legoktm pushed a commit that referenced this pull request Dec 11, 2023
fix ci after migrating our pip mirror to git-lfs
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.

Add support for starring and un-starring sources
2 participants