Multibit-Legacy / multibit-hd

Deprecated Bitcoin Wallet
https://multibit.org/blog/2017/07/26/multibit-shutdown.html
Other
173 stars 114 forks source link

Add support for basic Atom feed alert #769

Closed gary-rowe closed 8 years ago

gary-rowe commented 8 years ago

This is to allow us to communicate with users anonymously. Typically our blog articles will provide something that is of importance to MultiBit HD users and this is the most effective, and privacy conscious, method of achieving this.

Cold start

  1. User starts MultiBit HD
  2. Configuration checked to allow Atom feed reading on wallet unlock (active by default)
  3. Configuration checked for "Latest" UUID (not present)
  4. MultiBit.org Atom feed downloaded over HTTPS asynchronously
  5. Latest UUID in feed compared with local which triggers alert since it's not present
  6. Green alert shown with text: "Unread article on MultiBit.org: ''. Would you like to read it?
  7. If user selects the article the local browser is opened to the URL in the feed (no cookie/referrer)

Warm start

  1. User starts MultiBit HD
  2. Configuration checked to allow Atom feed reading on start up (active by default)
  3. Configuration checked for "Latest" UUID (present)
  4. MultiBit.org Atom feed downloaded over HTTPS asynchronously
  5. Latest UUID in feed compared with local which triggers alert only if different
  6. Green alert shown with text: "Unread article on MultiBit.org: ''. Would you like to read it?
  7. If user selects the article the local browser is opened to the URL in the feed (no cookie/referrer)

Disable news

  1. User selects Preferences | Appearance | Show news to "No" and clicks Apply
  2. Local configuration of Atom feed reading on start up is set to False
  3. Latest UUID is cleared to ensure a fresh alert if later set to "Yes"

Enable news

  1. User selects Preferences | Appearance | Show news to "Yes" and clicks Apply
  2. Local configuration of Atom feed reading on start up is set to True
  3. MultiBit.org Atom feed downloaded over HTTPS asynchronously
  4. Latest UUID in feed compared with local which triggers alert since it's not present
  5. Green alert shown with text: "Unread article on MultiBit.org: ''. Would you like to read it?
  6. If user selects the article the local browser is opened to the URL in the feed (no cookie/referrer)
jim618 commented 8 years ago

Looks good to me

jim618 commented 8 years ago

Just tried out the current code. It fetches the atom.xml correctly and shows the alert, which works.

Did you mean to invoke the check when you SWITCH the wallet ? I just switched the wallet and saw the alert - I think it only needs to run at start up.

jim618 commented 8 years ago

It might be better to change the alert text from: Unread article on MultiBit.org "some title or other". Would you like to read it ?

To

Would you like to read the unread article on MultiBit.org "some title or other" ?

That way a long title does not push off the action question into the ellipsis that will appear.

We'll also have to make sure our blog article titles are shorter rather than longer.

jim618 commented 8 years ago

Or, simpler: Would you like to read the article on MultiBit.org "some title or other" ?

gary-rowe commented 8 years ago

Not sure we need to change the text:

screen shot 2015-10-28 at 20 29 00

That title is reasonably descriptive and hardly makes a dent in the standard width.

Regarding the behaviour at switch (or rather unlock) this is intentional to cover the use case of a long-running opening with occasional wallet switches. In normal use the configuration will hide the Atom feed check so the behaviour you're seeing is indicative of a fresh article appearing during a wallet session.

gary-rowe commented 8 years ago

I've added the support code for the configuration and integrated it into the event handler in MainController. Instead of using the article UUID it was easier to store the URI since that was already present in the event and performs the same function. We shouldn't be changing article URIs once we've pushed the real feed so this is safe.

Here's a screen shot of the Appearance configuration screen:

screen shot 2015-10-28 at 21 10 13

Ready for review and close

jim618 commented 8 years ago

Tested with hard and soft wallets, cold and warm starts, turning news alerts off and on and it all works as expected.

Nice, unobtrusive information channel from us to users.

Closing.

jim618 commented 8 years ago

I have reopened this issue because the changes to the mbhd.yaml cause the released MBHD 0.1.4 to NOT load the configuration. They are not backwards compatible.

If you open 'v0.1.5' (i.e. the current code) and then try to open v0.1.4 it does not load the yaml and : asks for licence/ language/ forgets exchange rate etc. The first time I tried it it didn't even show the 'enter password' panel but the second time it did.

I thought there was code in the YAML loading so that adding extra variables didn't make it fall over. This needs investigating.

gary-rowe commented 8 years ago

Here's what's happening. At the top level we have Configuration which has the "catch-all" map that works for unknown properties, including their hierarchies, so long as the top level entry is also unknown. However, if we add a new property to a known top level entry such as Appearance then this "catch-all" stops working.

To fix it, we need to include the "catch-all" code in each known hierarchy (a bit messy) or accept that we're not fully backwards compatible.

jim618 commented 8 years ago

"not fully backwards compatible" = you lose all your settings and have to accept licence, choose language, set exchange and display options. It's a pain ! :-)

jim618 commented 8 years ago

Also, the already released 0.1.4 code will NOT be able to read any updated YAML, regardless of any additions added in the 0.1.5 code.

Perhaps it would be possible to move the parameters to somewhere that the 0.1.4 code can cope with.

All the more reason to have the 0.1.5 code clever enough not to fall over with future additions.

gary-rowe commented 8 years ago

Thought you'd come back with that, so I've added a bunch of code to handle unknown properties beyond the top level. To verify do the following:

Ready for review and close.

jim618 commented 8 years ago

Looks good. Checked visually that the othe configuration classes also have the same any() handling.

This will mean that copies of 0.1.4 won't understand 0.1.5 config files (or later) so users would have to set their config if they rolled back. But from 0.1.5 onwards we should be backwards compatible when adding properties.

Closing