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 enum kbdDisableBitmask to manage keyboard imput (#72) #73

Merged
merged 2 commits into from Jan 4, 2021

Conversation

H0neyBadger
Copy link

I'm keeping the PR in draft state to test the code correctly.
let me know if I have to change anything.
Thank you a lot

Copy link
Owner

@natinusala natinusala left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

The formatting looks funky at places but otherwise it does the job. I left some comments, I let you review them.

Once you are finished with the changes, before force pushing the branch, can you run ./scripts/format.sh --fix please?

library/include/borealis/list.hpp Outdated Show resolved Hide resolved
library/include/borealis/swkbd.hpp Outdated Show resolved Hide resolved
library/include/borealis/swkbd.hpp Outdated Show resolved Hide resolved
@H0neyBadger
Copy link
Author

Hello,
regarding the default value :

SwkbdKeyDisableBitmask_At | SwkbdKeyDisableBitmask_Percent | SwkbdKeyDisableBitmask_ForwardSlash | SwkbdKeyDisableBitmask_Backslash

Currently (in this PR), all filters are disabled. should I port this filter as default value for swkbd ?

@H0neyBadger H0neyBadger marked this pull request as draft November 5, 2020 20:45
@H0neyBadger H0neyBadger marked this pull request as ready for review November 7, 2020 09:47
@natinusala
Copy link
Owner

@WerWolv do you know why you disabled all "special" chars in swkbd by default? should we keep it that way or enable everything back?

@natinusala
Copy link
Owner

This is fine otherwise !

Let's wait for the answer from Wer and I'll try it once it's finished (and formatted 👀 ).

@H0neyBadger
Copy link
Author

Hi @natinusala,
thank you a lot for this review,
by fixing the format, you mean the multiline on function declaration ?
let me know if you have any suggestion. I will fix it asap

class Swkbd
{
  public:
    static bool openForText(std::function<void(std::string)> f, std::string headerText = "", std::string subText = "", int maxStringLength = 32, std::string initialText = "");
    static bool openForNumber(std::function<void(int)> f, std::string headerText = "", std::string subText = "", int maxStringLength = 32, std::string initialText = "", std::string leftButton = "", std::string rightButton = "");
    static bool openForText(std::function<void(std::string)> f, std::string headerText = "",
        std::string subText = "", int maxStringLength = 32, std::string initialText = "",
        int kbdDisableBitmask = KeyboardKeyDisableBitmask::KEYBOARD_DISABLE_NONE);
    static bool openForNumber(std::function<void(int)> f, std::string headerText = "",
        std::string subText = "", int maxStringLength = 32, std::string initialText = "",
        std::string leftButton = "", std::string rightButton = "",
        int kbdDisableBitmask = KeyboardKeyDisableBitmask::KEYBOARD_DISABLE_NONE);
};

I ran ./scripts/format.sh --fix with clang-format 11 (default version on my distrib)

@WerWolv
Copy link
Contributor

WerWolv commented Nov 7, 2020

@natinusala I think I just copied that function from my EdiZon codebase where I don't want these characters anywhere. You can absolutely change that behaviour

@natinusala
Copy link
Owner

Okay then @H0neyBadger I'd like everything to be available by default (so nothing disabled)

As for the formatting, we use clang-format, there is a script available : ./scripts/format.sh --fix

@H0neyBadger
Copy link
Author

yes, I already ran the fix ./scripts/format.sh --fix but with clang-format 11 instead of clang-format-8.
I do not know if there is big difference. (clang-tools-extra-11.0.0-1.fc33.x86_64)
I can use a container with clang-format-8 to compare the result if needed

@H0neyBadger
Copy link
Author

I just tested with ubundu's clang-format-8 and there is nothing to fix anymore (the current proposed patches are not related to this PR).
As far as I can see, the scripts/format.sh --fix is ok.
But let me know if I have to change some formats/syntax/names.

@natinusala
Copy link
Owner

So you're telling me that it's clang-format that splits the ListItem constructors in two lines? Same for openForNumber and friends

@H0neyBadger
Copy link
Author

H0neyBadger commented Nov 8, 2020

Hi, I did the split first but clang-format does not complain about it.
Should I put the ListItem construtors back in one line ?

@H0neyBadger
Copy link
Author

hi, all function declaration are back in one line (manually reverted).
let me know if I have to reformat something. thank you a lot for your help.

@H0neyBadger
Copy link
Author

Hello @natinusala,
sorry for asking. I know that's very difficult to find a spare moment for this.
Do you know if this change can be reviewed in a near future?

I would like to improve our "chiaki" project & user inputs.
But currently, I prefer to avoid workarounds in our code.
let me know If I have to implement this feature in our side.
thank you a lot

@natinusala
Copy link
Owner

Hi, I am so sorry, I missed the latest changes you made, I always get an email and check as soon as I can but I never got the emails for that...

The code looks fine! Can I request one last thing (sorry), to add some example ListItems to the example app to showcase the different swkbd modes?

Thanks!

@H0neyBadger
Copy link
Author

Hello, thank you a lot for your reply,
Ok no problem, I will add few examples ( like numbers and string ...)

btw some sxos users tend to report errors on swkbdShow. When they are using the regular homebrew channel the keyboard never show up. It affects all swkbdShow calls (not only borealis). the workaround is to run the homebrew channel in app mode, with full memory access.

@H0neyBadger
Copy link
Author

Hi, I just added an example.
Let me know if I have to change something.

resources/i18n/fr/main.json Outdated Show resolved Hide resolved
resources/i18n/fr/main.json Outdated Show resolved Hide resolved
@H0neyBadger
Copy link
Author

Nice catch. I've made the change requested

@natinusala natinusala merged commit cbdc1b6 into natinusala:master Jan 4, 2021
@H0neyBadger H0neyBadger deleted the feat/72 branch January 4, 2021 21:05
@natinusala
Copy link
Owner

Hey @H0neyBadger how are you?

I am currently in the process of changing the license of borealis from GPLv3 to Apache 2.0, and I need explicit permission from all contributors before continuing.

Do you allow me to change the license?

Thanks!

@H0neyBadger
Copy link
Author

H0neyBadger commented Feb 18, 2021 via email

@natinusala
Copy link
Owner

I'm great thank you😄

The reason is that GPLv3 is objectively a bad choice for a library, it's too limiting. And people told me that when I released it.

With yoga I made the library portable by abstracting away everything platform specific, but I realized that it's useless if the code stays GPL because nobody would be able to port it to their own platform.

I would like the library to grow and for that I need to allow usage for everyone, even commercial projects.

As for your question, it should not change anything for projects using borealis as long as the license is compatible with Apache 2.0. If it's not then you are stuck with the latest GPL version.

The change is only for yoga anyway, so I think that you're safe for now with chiaki.

@H0neyBadger
Copy link
Author

H0neyBadger commented Feb 19, 2021 via email

dragonflylee pushed a commit to dragonflylee/borealis that referenced this pull request Mar 6, 2024
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.

None yet

3 participants