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

Add More Fiat Currencies #3852

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

elibroftw
Copy link

@selsta , this just needs to be tested and then the tickers from #3362, can be added

@selsta
Copy link
Collaborator

selsta commented Mar 7, 2022

Could you remove the unrelated whitespace changes? Makes it easier to review.

@elibroftw
Copy link
Author

elibroftw commented Mar 7, 2022

@selsta
Copy link
Collaborator

selsta commented Mar 8, 2022

You can hide whitespace changes.

It pollutes git blame with unnecessary changes so usually we try to avoid whitespace changes but it's not that important here.

@elibroftw
Copy link
Author

elibroftw commented Mar 8, 2022

Don't merge this yet btw. Will need to test on local machine.

@selsta
Copy link
Collaborator

selsta commented Mar 8, 2022

I did some basic testing but more testing is always good.

@elibroftw
Copy link
Author

The dropdown worked fine? That was my only concern.

@selsta
Copy link
Collaborator

selsta commented Mar 8, 2022

yes, but I'll try to add a third currency

@elibroftw
Copy link
Author

Do you know if the Windows builds will be fixed soon? Or is it only failing on CI?

@selsta
Copy link
Collaborator

selsta commented Mar 8, 2022

I have asked luigi to merge #3849

@selsta
Copy link
Collaborator

selsta commented Mar 8, 2022

@elibroftw if you force push now Windows should be green

@elibroftw
Copy link
Author

You know if there's any future plans to support Visual Studio through vcpkg instead of requiring msys? Probably not right?

@selsta
Copy link
Collaborator

selsta commented Mar 8, 2022

It's a cross platform application and I don't know if Qt supports Visual Studio, so likely no. At least I won't add support for it as I'm not a Windows user.

@elibroftw
Copy link
Author

elibroftw commented Mar 8, 2022

I don't think the Windows build instructions are correct. I'm getting

maste@DESKTOP-F8TRQIV MINGW64 ~/monero-gui
$ cmake release-win64 -j4
bash: cmake: command not found

maste@DESKTOP-F8TRQIV MINGW64 ~/monero-gui
$ cmake release-win64 -j4
bash: cmake: command not found

maste@DESKTOP-F8TRQIV MINGW64 ~/monero-gui
$ ^C

maste@DESKTOP-F8TRQIV MINGW64 ~/monero-gui
$

After installing the libraries

@selsta
Copy link
Collaborator

selsta commented Mar 8, 2022

They say make release-win64 -j4 not cmake release-win64 -j4

@elibroftw
Copy link
Author

elibroftw commented Mar 8, 2022

I tried both, the former didn't work so I tried cmake. make was not found either.

@selsta
Copy link
Collaborator

selsta commented Mar 8, 2022

what is the output? also did you start the correct mingw64 console?

@elibroftw
Copy link
Author

elibroftw commented Mar 8, 2022

Yeah I started
image

Anyways, just merge whenever you feel like it since it worked it in the first place. Then I can create a PR for adding more currencies. I'll just test that one my laptop which I dual boot with Linux.

@elibroftw
Copy link
Author

Tested this on my laptop, it works!

@elibroftw
Copy link
Author

Can this get merged? Or should I add the fiat currencies in this PR itself?

@elibroftw
Copy link
Author

Helloooooooooooooooooooooooooooooooooo?

@selsta
Copy link
Collaborator

selsta commented Mar 15, 2022

Didn't have time to go through all emails yet.

To answer your question, however you want. You can wait for it to get merged or add extra currencies here too, ideally in a separate commit.

@elibroftw elibroftw force-pushed the master branch 4 times, most recently from 2f380f8 to a852d7c Compare March 15, 2022 13:21
@elibroftw
Copy link
Author

elibroftw commented Mar 15, 2022

The currencies work for coingecko and cryptocompare, but the kraken API is not getting called and kraken doesn't support XMR pairs other than with USD and EUR as far as I can tell

@elibroftw
Copy link
Author

And also the most important part is that the dropdown is not scrollable:
Screenshot from 2022-03-15 14-10-32

@selsta
Copy link
Collaborator

selsta commented Mar 27, 2022

Click on summary and then scroll down.

@elibroftw
Copy link
Author

elibroftw commented Mar 27, 2022

maste@Zealth:~/Downloads/docker-linux-static$./monero-wallet-gui
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
qt.qpa.plugin: Could not find the Qt platform plugin "wayland" in ""
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: xcb, xcb, minimal, offscreen, vnc.

Aborted (core dumped)

I guess this is why I can't run the stable build as well I installed from the store?

@selsta
Copy link
Collaborator

selsta commented Mar 27, 2022

I'm unfamiliar with wayland.

@reemuru
Copy link
Contributor

reemuru commented Mar 27, 2022

Ugh, I remember when Fedora first changed to wayland it was kinda buggy so I stayed on x11 as long as I could, but now it works fine. There are some apps which still misbehave though. @elibroftw are you able to run x11 desktop?

@elibroftw
Copy link
Author

elibroftw commented Mar 27, 2022

I know I can switch to X11, but I don't know why it still thinks I'm on wayland.


maste@Zealth:~/Downloads/docker-linux-static$ ./monero-wallet-gui 
qt.qpa.plugin: Could not find the Qt platform plugin "wayland" in ""
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: xcb, xcb, minimal, offscreen, vnc.

Aborted (core dumped)
maste@Zealth:~/Downloads/docker-linux-static$ head /etc/gdm3/custom.conf
# GDM configuration storage
#
# See /usr/share/gdm/gdm.schemas for a list of available options.

[daemon]
# Uncomment the line below to force the login screen to use Xorg
WaylandEnable=false

# Enabling automatic login
#  AutomaticLoginEnable = true
maste@Zealth:~/Downloads/docker-linux-static$ 

Screenshot from 2022-03-27 16-50-07

@elibroftw
Copy link
Author

It seems I can't run even the self compiled version anymore.

@elibroftw elibroftw marked this pull request as draft April 24, 2022 22:12
@WizardCornelius
Copy link

Hey guys, can I enquire whether this feature is still being worked on? It'd be amazing to use the wallet showing GBP fiat amount. Thanks for your time.

@elibroftw
Copy link
Author

I will try to work on it today. I stopped because I couldn't run the GUI anymore on my Linux machine but I installed a different distro recently plus I might be able to develop on windows itself.

@WizardCornelius
Copy link

Thanks mate, I really appreciate your time! I'm disabled, and the extra currency conversions I have to do using monero really exhaust me. It means a lot. Lmk if I can donate to support your work on this feature in some way.

@elibroftw
Copy link
Author

elibroftw commented Jan 6, 2023

Okay so I was able to take a look today and debugged some stuff. The issue is caused by the fact that the dropdown is a nested layout and combined with dynamically populating the fiat currencies listmodel. I honestly don't like that a ListModel is used when an array would suffice but maybe that's how QML works . We can either hard code the fiat currencies as ListElement and avoid the warning or rework the DropDown first. I suggest reworking the dropdown first as reemeru already has one started #3868 to fix the scrollable which isn't 100% usable.

@elibroftw
Copy link
Author

@WizardCornelius , for now I think you should use FeatherWallet. It's not the most optimal in terms of its layout, but it supports fiat pricing out of the box.

main.qml Show resolved Hide resolved
@elibroftw elibroftw force-pushed the master branch 2 times, most recently from 33858a3 to 4cbed50 Compare January 6, 2023 23:09
@elibroftw elibroftw marked this pull request as ready for review January 6, 2023 23:27
@elibroftw elibroftw force-pushed the master branch 2 times, most recently from 8de5f3d to bb8cecb Compare January 7, 2023 18:55
@selsta
Copy link
Collaborator

selsta commented Jan 8, 2023

Please remove the .vscode changes since they are unrelated to this PR.

@elibroftw
Copy link
Author

Done but I hope the vscode settings PR gets approved as it will improve productivity

fiatPriceProviderDropDown.currentIndex = i;
i += 1;
}

console.log('SettingsLayout loaded');
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

revert all the unnecessary white space changes to reduce the diffs of this PR please

Copy link
Author

Choose a reason for hiding this comment

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

Just let it be.

@Killercat103
Copy link

Any plans to potentially merge this?

@elibroftw
Copy link
Author

elibroftw commented Nov 13, 2023

@Killercat103 I'll probably end up creating a desktop wallet app myself before this PR gets merged. Reviewers are a bit too staunch when it comes to my PRs (3 in total).

main.qml Outdated Show resolved Hide resolved
@Killercat103
Copy link

Apparently the main issue is that the dropdown menu gets buggy when containing many currencies.

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.

6 participants