Add longpressthreshold setting to the chameleon settings page #565
Conversation
| @@ -1157,14 +1163,23 @@ class ChameleonCommunicator { | |||
| aLongPress = getButtonConfigType(resp[4]), | |||
| bLongPress = getButtonConfigType(resp[5]); | |||
|
|
|||
| // Default long press threshold is 1000ms | |||
| int longPressThreshold = 1000; | |||
There was a problem hiding this comment.
Return null if not possible to fetch
| } | ||
|
|
||
| Future<bool> setLongPressThreshold(int threshold) async { | ||
| try { |
There was a problem hiding this comment.
Do not catch exceptions in bridge code, you should catch them inside app code
| Future<bool> setLongPressThreshold(int threshold) async { | ||
| try { | ||
| // Enforce minimum value of 200ms | ||
| if (threshold < 200) { |
There was a problem hiding this comment.
Same there, should be capped on the app code, not library
And well, not silently change variables
There was a problem hiding this comment.
I know it's enforced by the firmware, but the user should get some feedback, not just set it to 50 only for it to either magically turn into 200 or stay the same.
There was a problem hiding this comment.
@Msprg "should be capped on the app code" means, that it should be capped on the app code, not that it shouldnt be capped. Have a slider or smthng and make the UI (app) code limit it. allows you to also give more descriptive error messages
|
|
||
| await sendCmd(ChameleonCommand.setLongPressThreshold, | ||
| data: Uint8List.fromList(u16ToBytes(threshold))); | ||
| return true; |
There was a problem hiding this comment.
Doesn't check real return, should return Future<void>
| } | ||
|
|
||
| Future<void> _updateLongPressThreshold() async { | ||
| if (!mounted) return; |
There was a problem hiding this comment.
Just no, there is FutureBuilder that fetches all info. Idk what prompt are you used, but output is just trash
| if (!appState.connector!.connected || appState.communicator == null) return; | ||
|
|
||
| try { | ||
| int threshold = await appState.communicator!.getLongPressThreshold(); |
There was a problem hiding this comment.
And well, getDeviceSettings returns threshold...
| @@ -291,6 +332,113 @@ class ChameleonSettingsState extends State<ChameleonSettings> { | |||
| appState.changesMade(); | |||
| }), | |||
| const SizedBox(height: 10), | |||
| Text("Buttons Long Press Threshold:", | |||
| return Row( | ||
| children: [ | ||
| Expanded( | ||
| child: TextFormField( |
There was a problem hiding this comment.
Slider should be better there, I doubt someone intentionally want to set threshold to exactly 5032 ms
There was a problem hiding this comment.
I dislike the slider as it offers either low precision and / or low range. I like to fine tune my values, and while I agree 5032ms is just stupid, values such as 210, 215, 225, 250, will be difficult to set. It could be supplemented by a slider, but not replaced. It's also good for accessibility.
There was a problem hiding this comment.
@Msprg have the classic arrangement of a slider next to a small textbox. best of both worlds. Maybe have the slider snap to some reasonable values (default value in the middle), i think that should be possible.
| builder: (BuildContext context) => AlertDialog( | ||
| title: const Text("Long Press Threshold"), | ||
| content: const Text( | ||
| "Sets the time in milliseconds for how long a button needs to be pressed to be considered a 'long press'.\n\nDefault: 1000ms (1 second)"), |
There was a problem hiding this comment.
"Long press threshold" is self explanatory
GameTec-live
added
stalled
|
(stalled) Waiting on RfidResearchGroup/ChameleonUltra#246 |
The code seems to be working, but it might need some adjustments as I honestly don't know what I'm doing lol.
Also, localization still needs to be added as the strings are hardcoded right now.
Settings version is now bumped to 6, so I changed how the comparison works a little to make it somewhat backwards compatible. Let's consider this a temporary fix? (but we all know how that usually ends, right?)
It might not be perfect, but dang does it work...