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

Propose to add inside the main view information about balance #30

Closed

Conversation

vincenzopalazzo
Copy link
Member

@vincenzopalazzo vincenzopalazzo commented Jul 26, 2020

Hi @lvaccaro,

With this PR, I want to propose adding more information, such as the balance information at the main view of lamp.

How discussed in some other PR, I think that is possible to use lightningd to calculate the balance of the wallet. However, the big problem is how to update and/or check if the balances are changed (At the moment I don't have a personal solution, I'm thinking yet).

The view at the moment when lightningd is running looks like the figure above

Selection_048

P.S: At the moment the label are static, sorry :)
P.S> At the moment is possible only receive the notification when the user funds a channel

@lvaccaro
Copy link
Collaborator

lvaccaro commented Aug 2, 2020

please, could you rebase?

@vincenzopalazzo
Copy link
Member Author

@lvaccaro all done

@lvaccaro lvaccaro changed the base branch from parse_uri to master August 2, 2020 22:11
@vincenzopalazzo vincenzopalazzo marked this pull request as ready for review August 4, 2020 13:59
@vincenzopalazzo
Copy link
Member Author

@lvaccaro I think that this PR is ready to revision it, I don't want to make a PR bigger than 20 files.

In sum, what I am proposing with this PR?

  • The main View contains the off-chain and on-chain balance
  • The draft of the notification system with the logging observer
  • A bug fix inside the Travis build travis

I hope this PR can help

@vincenzopalazzo vincenzopalazzo changed the title Draft propose to add inside the main view informations about balance Propose to add inside the main view information about balance Aug 9, 2020
@lvaccaro
Copy link
Collaborator

Hi, sorry to be late.. I tested the file log mechanism/notifications and they looks really good to me.
(I think some verbose message on logObserver could be removed)
At every daemon start, the relative log file is deleted (for clightning and tor) so it should not grow so much.

For UI balances, we need to re-organize some information
Screenshot_1597007331

@vincenzopalazzo
Copy link
Member Author

Hi @lvaccaro,

Sorry about the UI bug, I will work on it to fix the error.

Sorry again about that.

@vincenzopalazzo
Copy link
Member Author

In addition, about

At every daemon start, the relative log file is deleted (for clightning and tor) so it should not grow so much.

I don't test enough this, but on Google Pixel XL 4 and Android 10 I had a crash and the log file was 64 mb (too much information to analyze it on android phone, right?)

@vincenzopalazzo
Copy link
Member Author

@lvaccaro,

I looked inside the layout and I'm able to reproduce it with the small display, such as 4''. The problem look like be the space inside the display is not enough with the layout that I designed. I want have also your opinion on this layout and on a possible solution that we can add.

I tried to remove the lamp icon when the lightningd is running and use the button "shot down" inside the app bar but the layout looks like an empty layout, like the figure below

Selection_051

I'm happy to receive your feedback on this view.

In addition, I'm happy that do you like the system notification and it work for you.

(I think some verbose message on logObserver could be removed)

Do you mean the verbose message inside the android log?

@lvaccaro
Copy link
Collaborator

lvaccaro commented Aug 10, 2020

I am trying to reproduce the 64mb file error, and more testing, anyway it is a concept ack .
I mean the verbose log as "***** Actual line" happens really often.

There are 3 balances as you preview: offchain/onchain funds, moreover the amount you spend & receive in the channels. I am not able to suggest the best layout/strategy, but I don't dislike the main page of Zap app / Ligthning app (really essential) or Eclair app with small text of onchain/offchain balance. I am using Nexus One for testing.

vincenzopalazzo added a commit to vincenzopalazzo/lamp that referenced this pull request Aug 11, 2020
This is a limit cases but the app should be work well, to send money with some channel opened in the pass.

This is a in pass fix, I wil continue to work more on the PR clightning4j#30

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/lamp that referenced this pull request Aug 11, 2020
This is a limit cases but the app should be work well, to send money with some channel opened in the pass.

This is a in pass fix, I wil continue to work more on the PR clightning4j#30

Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo
Copy link
Member Author

Rebase with lvaccaro@7282c81 and made some change (not enough to review)

I am trying to reproduce the 64mb file error, and more testing,

I try to reproduce this and I understand that when the app are working for long time, the log file is big and with some use case for instance, if I want to see the log after shutdown, the text is too much and the app crash before to see the log activity. I can work on this problem after this PR.

I mean the verbose log as "***** Actual line" happens really often.

Removed, sorry about that

There are 3 balances as you preview: offchain/onchain funds, moreover, the amount you spend & receive in the channels.

In the last commit, I add an additional card to see the balance that you suggest here. I will work to support the small display.

Non sono in grado di suggerire il miglior layout / strategia, ma non mi dispiace la pagina principale dell'app Zap / App Ligthning (davvero essenziale) o dell'app Eclair con un piccolo testo di bilanciamento onchain / offchain.

I like your suggestion, I think that is a cool idea to use two fragments in the same main activity to switch it and make the UI a little cleaner. But this requires a little refactoring and at the moment I want (if you agree) try to make a subsistem a little more strong and try to support all type of notify and only after improve the UI like the actual lightning wallet

@lvaccaro
Copy link
Collaborator

I full agree with your consideration, maybe it is better build something stable for now and postpone main ui refactor.

Then I am ok to have balance, maybe a little smaller.

Related to the crash, I have just merged the lightning_v0.9.0 branch (I hope it is not related to this).
I really appreciate a more technical crash reports, maybe to exclude some others reason (as esplora or connectivity problem) also not in this MR.

If it could help, you could try to retrieve the linghtningd.log file from fileDir() programmatically before it was cleaned by a fresh lightningd starts.

@vincenzopalazzo
Copy link
Member Author

vincenzopalazzo commented Aug 11, 2020

Hi @lvaccaro,

I will work on UI, I will update tomorrow 👍

About the crash, I founded the problem and I pushed a solution, at app level, but I think is good to understand what happened inside lightningd.

In sum, after the restart of lightning without tor the not have 0 addressed where it can bind to make the node ready to receive the request of connection.

More here https://github.com/lvaccaro/lamp/pull/35

If you want discuss why the addresses array is empty I'm ready to help :)

Update

I think that I answered to different crash, I mean the crash without tor and inside this conversation is mentioned only the log crash.
The log crash is only inside the app, too much line loaded inside the text container

Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo
Copy link
Member Author

Sorry the confusion on this PR.

I release the new proposal about the UI interface, I think is a good point where to start to test the notify and see how to work with this implementation.

Sorry again about the confusion

This is the actual app version

@lvaccaro
Copy link
Collaborator

Hi, balances looks good also in the emulator.

I think the UI contains twice the same actions, accessible and displayed in different ways:

  • read invoice:
  1. from send button: manually typing a invoice (I think that will never happen) or copy from clipboard
  2. from camera floating bar: scan qr code or copy from clipboard
  • build invoice:
  1. from menu: sats icon
  2. from receive button: open same dialog as before

Related to PayViewActivity maybe if you could split the commis we can continue the discussion in another MR:

  • looks defined with an intent-filter in the manifest for launcher
  • when press "pay" button it doesn't show the decoded information
  • there is no qr code reader

PS: send / receive are common words used both for onchain & offchain tx. Maybe it is only my opinion but I prefer pay (instead send) and new invoice (instead receive).

I am pretty sure most of the previous UI will be overwritten as soon as an organic design will be available, but I prefer to merge the file logger contained here asap.

@vincenzopalazzo
Copy link
Member Author

Split in #38 and #39

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