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

applets/swkbd: Software Keyboard Implementation #3850

Merged
merged 6 commits into from Jul 17, 2018

Conversation

Projects
None yet
8 participants
@zhaowenlan1779
Member

zhaowenlan1779 commented Jun 20, 2018

Wow, so many additions.
Hmm, this is yet another swkbd implementation.

But, this also includes a frontend applet interface by @jroweboy. For this part I only fixed typos. Now removed, but core/frontend/swkbd part is mostly written by @jroweboy.
Also added log classes: Applet and Applet_SWKBD.
I guess the code is fairly easy to understand, and I won't have to explain my designs, so I'm just putting a screenshot here:
image

Buttons, custom texts, validations are properly implemented.

Fixes #1583, fix #2095, fix #3382, fix #3346

fix #3477 by last commit


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Jun 20, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-20T12:10:19Z: 964602d...zhaowenlan1779:a0d8891c40a10966a153f1256c49122beda2f103

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:swkbd branch from a0d8891 to 4b1bc1c Jun 20, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jun 20, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-20T12:11:26Z: 964602d...zhaowenlan1779:4b1bc1cd6306290e3ce0b32269d3ec2ccd0cccab

// some games seem to check for a hardcoded 2
config.return_code = 2;
config.text_length = 6;
config.text_length = static_cast<u16>(text.size() + 1);

This comment has been minimized.

@wwylele

wwylele Jun 20, 2018

Member

IIRC I have tested that text_length doesn't include the terminating '\0', and the length 6 in the old code was a mistake.

@neobrain

This comment has been minimized.

Member

neobrain commented Jun 20, 2018

What will happen if the user enters a character that cannot usually be entered by the swkbd on the 3DS, like for instance 💩 ?

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Jun 20, 2018

@neobrain I tested that and found the app just happily displayed a ?. Wise point and I will (probably) add that to the validator.

frontend_config.multiline_mode = config.multiline;
frontend_config.max_text_length = config.max_text_length;
frontend_config.max_digits = config.max_digits;
std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t> convert;

This comment has been minimized.

@wwylele

wwylele Jun 20, 2018

Member

Use Common::UTF16ToUTF8 and related functions, because the standard library (used to) have problems. We want to unify the codecvt code and update them according to the standard library status all together

frontend_config.max_digits = config.max_digits;
std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t> convert;
frontend_config.hint_text =
convert.to_bytes(reinterpret_cast<const char16_t*>(config.hint_text.data()));

This comment has been minimized.

@wwylele

wwylele Jun 20, 2018

Member

Avoid reinterpret_cast and use std::memcpy to a buffer instead.

public:
explicit SoftwareKeyboard() : AppletInterface() {}
void Setup(const AppletConfig* config) override {
this->config = KeyboardConfig(*static_cast<const KeyboardConfig*>(config));

This comment has been minimized.

@wwylele

wwylele Jun 20, 2018

Member

This static_cast probably reveals a potential design problem. First of all it is obviously not type-safe, just like the old-school void*. Actually you would expect that every applet type would have to static_cast this because they want different configuration types => You actually want to make different Setup signature for different applet types => Then you probably want applets of different types to be, well, completely unrelated types/classes. This indeed would break your homogeneous container registered_applets, however it is not really necessary as you can totally just have SoftwareKeyboard registered_softwarekeybord; MiiSelector registered_miiselector; ... (And probably only these two applets worth GUI implementation)

In summary: the base class AppletInterface is really unnecessary as

  • It makes you do more unsafe casting, and makes it awkward to do anything related to a specific applet type.
  • It doesn't provide useful implementation to be shared with all applets
  • You don't benefit anything from polymorphism, as the polymorphic dispatch already happened in core/hle/service/applet_manager -> core/hle/applet
#pragma once
#include <codecvt>
#include <locale>

This comment has been minimized.

@wwylele

wwylele Jun 20, 2018

Member

Where are these two used?

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:swkbd branch from 4b1bc1c to 78f35e2 Jun 20, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jun 20, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-20T12:57:18Z: 964602d...zhaowenlan1779:78f35e22f70c12dd4ad12733d50c64e41d2635d6

@cluezbot

This comment has been minimized.

cluezbot commented Jun 20, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-20T14:01:31Z: 964602d...zhaowenlan1779:9266d29e9df0f08e12913b4da8cb3fa9b0296f3a

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Jun 20, 2018

MSVC is failing, but I will have to solve them after this Saturday.

@Subv

Note that this is still an incomplete implementation, but I don't have anything against it considering it's just the frontend side of things.

There still needs to be a better implementation of the Game<->Applet communication protocol, it's not as simple as it's currently implemented. Games can continue to communicate with the swkbd in a ping-pong fashion, for example if a custom validation function is provided by the game (see the libctru swkbd example).

@@ -1456,6 +1458,10 @@ int main(int argc, char* argv[]) {
Camera::RegisterFactory("qt", std::make_unique<Camera::QtMultimediaCameraFactory>());
Camera::QtMultimediaCameraHandler::Init();
// Register frontend applets
Frontend::RegisterDefaultApplets();
Frontend::RegisterSoftwareKeyboard(std::make_shared<QtKeyboard>(&main_window));

This comment has been minimized.

@Subv

Subv Jun 20, 2018

Member

Use a reference if you're going to take the address of a local variable

@cluezbot

This comment has been minimized.

cluezbot commented Jun 23, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-23T05:31:32Z: 964602d...zhaowenlan1779:dc19b28d01e8405bc874946a5dcfa8ae0e957290

@zhaowenlan1779 zhaowenlan1779 changed the title from applet/swkbd: Software Keyboard implementation, and frontend applet interface to applets/swkbd: Software Keyboard Implementation Jun 23, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jun 23, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-23T07:08:10Z: f50e505...zhaowenlan1779:066c3d07f75ce64e16f6f01ce3fa0a3534d54a03

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:swkbd branch from dc19b28 to 066c3d0 Jun 23, 2018

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Jun 23, 2018

Rebased commits.

/// Index of the buttons
u8 ok_id;
u8 forgot_id = 1;

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

forgot_id and cancel_id can be static constexpr given they're not modified.

ValidationError SoftwareKeyboard::ValidateFilters(const std::string& input) {
if (config.filters.prevent_digit) {
if (std::any_of(input.begin(), input.end(),

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

<algorithm> needs to be included.

return ValidationError::MaxLengthExceeded;
}
auto is_blank = [&] {

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

const auto

return std::all_of(input.begin(), input.end(),
[](unsigned char c) { return std::isspace(c); });
};
auto is_empty = [&] { return input.empty(); };

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

ditto. Also is this really better than just typing input.empty()?

data = {text, button};
}
std::shared_ptr<SoftwareKeyboard> registered_swkbd;

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

Please don't implement globals like this. This either belongs in the front-end itself, or in the System class.

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 23, 2018

Member

I'm just doing it like how the camera is registered, and I do not know how to put it in the frontend itself. (How can I get it then?)

BTW this isn't going to be a global; it exists only in the cpp file.

Put that in the System class? I do not like the idea, as this is and should be in the namespace of Frontend, I think.

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

I disagree entirely.

  1. It's not static so it has external linkage (that still doesn't resolve other issues).
  2. It makes it a pain to launch more than one instance of the core or make library portion of the emulator a proper library in the future. I don't think making the core worse in this regard is a good idea (other globals existing should not be seen as an encouragement to do the same).
  3. Just because something else is haphazard doesn't mean other things should also be that haphazard as well.
  4. Even if it's file-scope, it's still functionally a global from a behavioral point of view.

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

Fundamentally if an instance of something is providing behavior that the system provides, it belongs in the System class, full-stop.

char16_t display_text[65]; ///< Text to display when asking the user for input
/// Keyboard password modes.
enum class SoftwareKeyboardPasswordMode : u32 {
NONE = 0, ///< Characters are not concealed.

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

Ditto. = 0 can also be omitted here.

u32 default_text_offset; ///< Offset of the default text in the output SharedMemory
/// Keyboard filter callback return values.
enum class SoftwareKeyboardCallbackResult : u32 {
OK = 0, ///< Specifies that the input is valid.

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

Ditto. = 0 can also be omitted here.

INSERT_PADDING_WORDS(0x3);
/// Keyboard return values.
enum class SoftwareKeyboardResult : s32 {
NONE = -1, ///< Dummy/unused.

This comment has been minimized.

@lioncash
std::memcpy(buffer.data(), config.hint_text.data(), config.hint_text.size() * sizeof(u16));
frontend_config.hint_text = Common::UTF16ToUTF8(buffer);
frontend_config.has_custom_button_text =
!std::all_of(config.button_text.begin(), config.button_text.end(),

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

<algorithm> needs to be included.

frontend_config.max_text_length = config.max_text_length;
frontend_config.max_digits = config.max_digits;
std::u16string buffer;
buffer.resize(config.hint_text.size());

This comment has been minimized.

@lioncash

lioncash Jun 23, 2018

Member

std::utf16string buffer(config.hint_text.size(), 0);

The resize isn't necessary.

@cluezbot

This comment has been minimized.

cluezbot commented Jun 23, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-23T11:24:01Z: f50e505...zhaowenlan1779:a2f2dd7c5fa5f55564290437d451361075c6af59

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Jun 23, 2018

@lioncash I did not change the line of std::shared_ptr<SoftwareKeyboard> registered_swkbd;. Please see my comment above (for strange reasons the conversation is hidden as outdated)

@neobrain Can you please tell me what characters can be entered?

@cluezbot

This comment has been minimized.

cluezbot commented Jun 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-24T01:03:01Z: f50e505...zhaowenlan1779:0ab5107fba7301a13d7939bbae166af34fdf3605

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:swkbd branch from 0ab5107 to 54bf035 Jun 24, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jun 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-24T01:05:19Z: 689977b...zhaowenlan1779:54bf035f81cb9d48cae1defc6c611b6606931c3c

Memory::ZeroBlock(info.address_left, info.stride * 320);
Service::GSP::SetBufferSwap(1, info);
// TODO(Subv): Draw the HLE keyboard, for now just do nothing

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 24, 2018

Member

Explanation: The implementation of the function is useless, for all cases now:

  1. You don't need to wait for DefaultCitraKeyboard, so you will hardly see the effect
  2. QtKeyboard blocks before returning from Setup, so frame display will not be updated until input dialog has finished. The black screen will still be displayed, but it is a splash after input dialog finishes, which looks quite stupid.

This comment has been minimized.

@jroweboy

jroweboy Jun 27, 2018

Member

QtKeyboard blocks before returning from Setup

This doesn't sound correct. The keyboard shouldn't block on setup since 3ds applets don't either. I suppose I'm fine with this behavior for now, but its not really acceptable as a long term solution especially since we'll need the game still running to have them run the callbacks

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Jun 24, 2018

@lioncash Moved registered_swkbd to System

@lioncash

Left a review about minor other things that I meant to do in the previous review (sorry for not seeing them sooner)

@@ -71,6 +195,8 @@ class SoftwareKeyboard final : public Applet {
void Finalize();
private:
Frontend::KeyboardConfig ToFrontendConfig(SoftwareKeyboardConfig config);

This comment has been minimized.

@lioncash

lioncash Jun 24, 2018

Member

config should likely be a const reference, otherwise this does quite a large copy. The member function can likely be a const member function as well

};
/// Default English button text mappings. Frontends may need to copy this to internationalize it.
static const char* BUTTON_OKAY = "Ok";

This comment has been minimized.

@lioncash

lioncash Jun 24, 2018

Member

These can just be:

constexpr char name[] = "Text"

const file-scope variables are static by default. Using arrays instead of pointers marginally saves executable space (a pointer address doesn't need to be stored in the executable alongside the string data, only the string data itself, so it's basically free).

#include <QVBoxLayout>
#include "citra_qt/applets/swkbd.h"
QtKeyboardValidator::QtKeyboardValidator(QtKeyboard* keyboard) : keyboard(keyboard) {}

This comment has been minimized.

@lioncash

lioncash Jun 24, 2018

Member

This will cause a variable shadowing warning. The parameter should be keyboard_ or something else

}
}
QtKeyboardDialog::QtKeyboardDialog(QWidget* parent, QtKeyboard* keyboard) : keyboard(keyboard) {

This comment has been minimized.

@lioncash

lioncash Jun 24, 2018

Member

Ditto about keyboard

QMessageBox::critical(this, tr("Validation error"), VALIDATION_ERROR_MESSAGES.at(error));
}
QtKeyboard::QtKeyboard(QWidget& parent) : parent(parent) {}

This comment has been minimized.

@lioncash

lioncash Jun 24, 2018

Member

Ditto about variable shadowing

@@ -199,6 +199,10 @@ const Service::SM::ServiceManager& System::ServiceManager() const {
return *service_manager;
}
void System::RegisterSoftwareKeyboard(std::shared_ptr<Frontend::SoftwareKeyboard> swkbd) {
registered_swkbd = swkbd;

This comment has been minimized.

@lioncash

lioncash Jun 24, 2018

Member

This can be registered_swkbd = std::move(swkbd); to avoid an unnecessary atomic reference count increment and decrement

@cluezbot

This comment has been minimized.

cluezbot commented Jun 26, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-26T11:15:11Z: 689977b...zhaowenlan1779:4e5beb8cf2cc0c088b963ea9dbe5ef9d769e9e7a

@cluezbot

This comment has been minimized.

cluezbot commented Jun 26, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-26T11:20:41Z: 0a34054...zhaowenlan1779:9b80877ad5237d8bdb8af9f2a1fd1680ebbb7406

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:swkbd branch from 4e5beb8 to 9b80877 Jun 26, 2018

jroweboy and others added some commits Mar 22, 2018

frontend/applets: frontend swkbd base
Original commits by @jroweboy:

* Rebase out the other commit

* changing branches

* More work on stuff and things ecks DEE

Changes by @zhaowenlan1779:

* Removed #include of result.h
frontend/applets: misc fixes
* Renamed applet to applets

* Added log classes Applet and Applet.SWKBD

* Fixes to get it compile

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:swkbd branch from 9b80877 to 44b15a8 Jun 30, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jun 30, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-06-30T00:14:59Z: f9a89ff...zhaowenlan1779:44b15a897b90ed2273bc39055e9c7f7eaa662df4

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:swkbd branch from 44b15a8 to 05334e6 Jul 2, 2018

@BreadFish64

This comment has been minimized.

Contributor

BreadFish64 commented Jul 9, 2018

is it possible to limit the number of characters in the textbox, instead of showing a popup when there's too many characters

zhaowenlan1779 added some commits Jun 20, 2018

citra_qt/applets/swkbd: QtKeyboard and misc fixes
* Addressed comments and removed the applet interface

* swkbd: address @lioncash's comments

* core: more fixes

** Moved registered_swkbd to System

** Removed an usused virtual

** Removed functionality of DrawScreenKeyboard

** Removed src/core/settings.h change

* swkbd: address @lioncash's 2nd review

* swkbd: update logging macro

* QtKeyboard: Make dialog modal and hide help

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:swkbd branch from 05334e6 to dcaf4a8 Jul 10, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jul 10, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-10T05:05:40Z: 610acf2...zhaowenlan1779:dcaf4a8e83a7c7fea2b1beafedb2ff1fe78454f3

@cluezbot

This comment has been minimized.

cluezbot commented Jul 17, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-17T14:43:34Z: 610acf2...zhaowenlan1779:b54e3b7aa969c2e98f11babeaf23268ce1bbfbf1

@jroweboy jroweboy merged commit bf6da61 into citra-emu:master Jul 17, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@zhaowenlan1779 zhaowenlan1779 deleted the zhaowenlan1779:swkbd branch Jul 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment