Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 initial browser application #189
base: master
Are you sure you want to change the base?
Add initial browser application #189
Changes from all commits
08dd4b1
96f8d11
f67d830
16a0812
2eda0e1
acf94b9
f8d1bc5
ac463cb
83f40b5
a046c42
4174f3d
5a09b2f
a8345a8
1f532b7
c33ff53
627b582
bebf1c4
509e413
9860a4e
12d0355
385e458
63fccc1
071a0a7
9ba4abc
cd949d1
a4ca22c
7aaa2b2
3f5ad0c
346cf47
e434d43
d0404f4
b01e7f3
bd6b061
84012d1
f669fa8
d1a3703
75f2592
ef504f0
fa35c3e
cd4c969
cc67c9b
7d30586
525a77c
156c4b1
5b46e7b
9d8514e
a62f708
0b5fb75
3791244
47f8c77
d865344
6711466
268daf1
dbf282d
b5e0953
f9731c8
950ef30
8e8037d
2adbb83
6573bb6
596c270
ea51ab9
29c9b26
83d0869
6516719
3da3981
b9c6063
0339324
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check notice on line 25 in applications/web-browser/controller.h
codefactor.io / CodeFactor
applications/web-browser/controller.h#L25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace string literal exception with a standard exception
Using string literals in
throw
statements is not recommended. It's better to throw exceptions derived fromstd::exception
for proper exception handling. Consider throwing astd::runtime_error
or a custom exception class.Suggested change:
Add the necessary include at the top of your file:
+#include <stdexcept>
And update the throw statement:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace string literal exception with a standard exception
In the
migrate
function, throwing a string literal is not recommended. Use a standard exception derived fromstd::exception
.Suggested change:
Ensure you have included
<stdexcept>
at the top of your file.Update the throw statement:
📝 Committable suggestion
Check notice on line 13 in applications/web-browser/keyboardhandler.h
codefactor.io / CodeFactor
applications/web-browser/keyboardhandler.h#L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine 'key' and 'modifier' using bitwise OR '|' instead of addition
When creating a
QKeySequence
, use bitwise OR to combine the key and modifiers correctly.Update the line as follows:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me to documentation on why this is preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rate Limit Exceeded
@Eeems have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 47 seconds before sending another message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me to documentation on why this is preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rate Limit Exceeded
@Eeems have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 18 seconds before sending another message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rate Limit Exceeded
@Eeems have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 21 seconds before sending another message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me to documentation on why this is preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Eeems
Certainly! In Qt, when combining a key code with modifiers to create a
QKeySequence
, it's recommended to use the bitwise OR|
operator instead of addition+
. This is because both key codes and modifiers are represented as bit flags (enumerations), and the bitwise OR operation correctly merges these flags without altering their individual bits.Using addition may lead to unintended results since it can produce key codes that don't correspond to any valid key or modifier combination. The bitwise OR ensures that each flag is preserved and combined appropriately.
For more details, you can refer to the Qt documentation on
QKeySequence
andQt::KeyboardModifier
, which illustrate how to combine keys and modifiers using bitwise operations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the documentation, this method takes a string to define the key sequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Eeems
You are correct, and I apologize for the confusion earlier. The
QKeySequence
constructor typically expects aQString
or a standard key, and combining thekey
andmodifier
using bitwise OR may not produce the intended result.To properly display the key sequence in your debug output, you might consider formatting the key and modifiers separately. For example:
qDebug() << "Char pressed: Key = " << QKeySequence(key).toString() << ", Modifiers = " << modifier;
This way, you can see the key and modifiers distinctly.
Again, sorry for any confusion caused.
✏️ Learnings added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use bitwise OR for combining key and modifier.
The
charPress
method looks good overall, but there's one improvement to make:On line 29, use bitwise OR (
|
) instead of addition (+
) to combinekey
andmodifier
:This ensures correct combination of key and modifier flags.
The rest of the method's logic for handling character input and null characters is well-implemented.
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use bitwise OR '|' to combine 'key' and 'modifier' in 'QKeySequence'
Adding 'key' and 'modifier' may not correctly represent the key sequence with modifiers.
Modify the code:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use bitwise OR for combining key and modifier in default case.
The
keyPress
method is well-implemented, but there's one improvement to make:In the switch statement's default case (line 56), use bitwise OR (
|
) instead of addition (+
) to combinekey
andmodifier
:This ensures correct combination of key and modifier flags.
The rest of the method, including the separate handling of modifier keys, is well-structured and logically sound.
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct invalid addition of QString and Qt::KeyboardModifiers in debug output.
The
stringPress
method is well-implemented, but there's an issue in the debug output:On line 77, the addition of
text
(QString) andmodifier
(Qt::KeyboardModifiers) is invalid and won't compile. Consider modifying the debug output:The rest of the method, including the check for editable targets and the text insertion logic, is well-structured and correct.
📝 Committable suggestion