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

Embedding Legal Notices as Embedded Resources. #1661

Merged
merged 10 commits into from Jun 30, 2019

Conversation

Projects
None yet
7 participants
@jmacato
Copy link
Contributor

commented Jun 29, 2019

Allows for the texts to be more easier to handle on git diffs whenever they are changed. Also hardcoding long strings isn't ideal i think.

jmacato added some commits Jun 29, 2019

@jmacato jmacato marked this pull request as ready for review Jun 29, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2019

@hgergoil @bharmat Is this ok to move around these file without changing anything in them or that's somehow problematic legally speaking?

@jmacato jmacato force-pushed the jmacato:embed-notices-as-assets branch from 9ea8f7c to edfb715 Jun 29, 2019

@bharmat

This comment has been minimized.

Copy link

commented Jun 30, 2019

I do not see any problem with it from legal point of view. Go ahead.

nopara73 added some commits Jun 30, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

Read blocks the UI thread in the constructor.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

Fixed: 406188f

.ToObservable()
.ObserveOn(RxApp.MainThreadScheduler)
.Subscribe(x => Text = x)
.DisposeWith(Disposables);

This comment has been minimized.

Copy link
@nopara73

nopara73 Jun 30, 2019

Collaborator

@lontivero @molnard I think this Task.ToObservable is super useful. Can you take a note of this pattern?

This comment has been minimized.

Copy link
@nopara73

nopara73 Jun 30, 2019

Collaborator

When you want to run an async function in the ViewModel's constructor, then make it ToObservable and subscribe to its result on the UI thread. Also make sure to dispose (In OnClose usually.)

This comment has been minimized.

Copy link
@lontivero

lontivero Jul 1, 2019

Collaborator

ACK. Why is this pattern better than simply using the old continuations with ContinueWith?

This comment has been minimized.

Copy link
@molnard

molnard Jul 2, 2019

Collaborator

Very beautiful in this particular case but not a silver bullet for every place - notice taken!
@lontivero with ContinueWith wouldn't we need a Task.Run or an await?

This comment has been minimized.

Copy link
@lontivero

lontivero Jul 2, 2019

Collaborator

ContinueWith returns a Task that you can await or not. Here, for example, we are not awaiting the task returned by the LoadDocumentAsync method.

This comment has been minimized.

@nopara73 nopara73 merged commit 7d4ea4b into zkSNACKs:master Jun 30, 2019

2 of 4 checks passed

Wasabi.Linux in progress
Details
Wasabi.Windows in progress
Details
CodeFactor No issues found.
Details
Wasabi.Osx #20190630.22 succeeded
Details

@jmacato jmacato deleted the jmacato:embed-notices-as-assets branch Jul 1, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 1, 2019

A bit of meta topic. 506 lines will come down from your score of the Contribution Game, since that's how many files were removed then added as text files.

https://github.com/zkSNACKs/WalletWasabi/blob/master/WalletWasabi.Documentation/Guides/20190617ContributionGame.md

Update (2019-06-28)

PR #1661 moved around large files. 49+154+50 lines have been removed and added, so 506 lines will come down from @jmacato's score.

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 1, 2019

We are loosing part of the text.

Privacy Policy

image

Terms and Conditions

image

Legal Issues works well in my case.


Update:

The way to reproduce it is by resizing the window. This same problem happened before and I didn't find a clean solution, that's why we add empty lines at the end of the texts so, in case the text is cut, it was removing those empty lines from the end.

@hgergoil

This comment has been minimized.

Copy link

commented Jul 2, 2019

I would be happy if we aren't loosing part of the text.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2019

On Windows everything works fine.

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Jul 3, 2019

@jmacato I have tested again, this time with latest master commit (e366e93) and it doesn't look okay in my Linux after resizing it. Could you take a look at it, please?

@MaxHillebrand

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

I'm loosing some of the bottom text in Debian with i3 window manager, but only when the size of the window get's smaller...

image

@jmacato

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@lontivero yeah can confirm it does that weird cut-off when resized. Perhaps we could just add back your 100 newline workaround as a temporary measure. I think this could be an avalonia bug but i'll have to do a repro to verify

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.