Re: [pve-devel] [PATCH pve_flutter_frontend v4] ui: enable noVNC console on iOS

[Dominik Csapak <d.csapak@proxmox.com>]

hi,

thanks for sending the patch again without dart format changes (mostly,
see my comment on the last hunk)

while the changes themselves look ok to me, i would have liked it more
for the different changes to be different patches

e.g. adding the appbar is ok, but does not have anything to do with
adding ios support (that change is only a single line FWICT)

also refactoring is also always good, but having those commits
separate makes it much more easy to see what is actually changed.

aside from that, there are a few comments inline

On 8/14/25 11:32, Shan Shaji wrote:
> The noVNC console was disabled in iOS and was only available in android
> 
> To fix the issue enabled the noVNC console view on iOS devices. The
> changes also includes a refactor of the function responsible for
> displaying the webview.
> 
> Additionally, the `PveAppBar` has been added to the console view
> to allow users to easily close the view. To avoid the body of the
> scaffold to resize when the keyboard pops up set
> `resizeToAvoidBottomInset` [0] to false.
> 
> - [0] https://api.flutter.dev/flutter/material/Scaffold/resizeToAvoidBottomInset.html
> 
> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
> ---
> 
>   changes since v3:
>   - Remove dart formatting changes.
>   - Updated commit message.
> 
>   changes since v2:
>   - Set `resizeToAvoidBottomInset` to false to avoid the scaffold body
>     to resize automatically when the keyboard pops up.
>   - Update commit message.
> 
>   changes since v1:
>   - Rebased with master.
>   - Updated commit message.
> 
>   lib/widgets/pve_console_menu_widget.dart | 68 +++++++++++-------------
>   1 file changed, 30 insertions(+), 38 deletions(-)
> 
> diff --git a/lib/widgets/pve_console_menu_widget.dart b/lib/widgets/pve_console_menu_widget.dart
> index 473595d..ab98ce4 100644
> --- a/lib/widgets/pve_console_menu_widget.dart
> +++ b/lib/widgets/pve_console_menu_widget.dart
> @@ -9,6 +9,7 @@ import 'package:proxmox_dart_api_client/proxmox_dart_api_client.dart';
>   import 'package:proxmox_login_manager/proxmox_general_settings_model.dart';
>   import 'package:flutter_inappwebview/flutter_inappwebview.dart';
>   import 'package:crypto/crypto.dart';
> +import 'package:pve_flutter_frontend/widgets/pve_app_bar.dart';
>   
>   class PveConsoleMenu extends StatelessWidget {
>     static const platform =
> @@ -97,45 +98,18 @@ class PveConsoleMenu extends StatelessWidget {
>                     }
>                   },
>                 ),
> -            if (Platform.isAndroid) // web_view is only available for mobile :(
> +            // web_view is only available for mobile :(
> +            // xterm.js doesn't work that well on mobile

i would leave that comment on the novnc console part, since it does not
relate to the line below regarding android/ios

> +            if (Platform.isAndroid || Platform.isIOS)
>                 ListTile(
>                   title: const Text(
> -                  //type == "qemu" ? "noVNC Console" : "xterm.js Console",
> -                  "noVNC Console", // xterm.js doesn't work that well on mobile
> +                  "noVNC Console",
>                     style: TextStyle(fontWeight: FontWeight.bold),
>                   ),
>                   subtitle: const Text("Open console view"),
> -                onTap: () async {
> -                  if (Platform.isAndroid) {
> -                    if (['qemu', 'lxc'].contains(type)) {
> -                      SystemChrome.setEnabledSystemUIMode(
> -                          SystemUiMode.immersive);
> -                      Navigator.of(context)
> -                          .push(_createHTMLConsoleRoute())
> -                          .then((completion) {
> -                        SystemChrome.setEnabledSystemUIMode(
> -                            SystemUiMode.edgeToEdge,
> -                            overlays: [
> -                              SystemUiOverlay.top,
> -                              SystemUiOverlay.bottom
> -                            ]);
> -                      });
> -                    } else if (type == 'node') {
> -                      SystemChrome.setEnabledSystemUIMode(
> -                          SystemUiMode.immersive);
> -                      Navigator.of(context)
> -                          .push(_createHTMLConsoleRoute())
> -                          .then((completion) {
> -                        SystemChrome.setEnabledSystemUIMode(
> -                            SystemUiMode.edgeToEdge,
> -                            overlays: [
> -                              SystemUiOverlay.top,
> -                              SystemUiOverlay.bottom
> -                            ]);
> -                      });
> -                    }
> -                  } else {
> -                    print('not implemented for current platform');
> +                onTap: () {
> +                  if (['qemu', 'lxc'].contains(type) || type == 'node') {

this check now looks weird.

i'd either prefer `['qemu', 'lxc', 'node'].contains(type)` or
something like

type == 'qemu' || type == 'lxc' || type == 'node'

but please don't mix both ;)


> +                    _openNoVncConsole(context);
>                     }
>                   },
>                 ),
> @@ -145,6 +119,18 @@ class PveConsoleMenu extends StatelessWidget {
>       );
>     }
>   
> +  Future<void> _openNoVncConsole(BuildContext context) async {
> +    SystemChrome.setEnabledSystemUIMode(SystemUiMode.immersive);
> +    await Navigator.push(context, _createHTMLConsoleRoute());
> +    SystemChrome.setEnabledSystemUIMode(
> +      SystemUiMode.edgeToEdge,
> +      overlays: [
> +        SystemUiOverlay.top,
> +        SystemUiOverlay.bottom,
> +      ],
> +    );
> +  }
> +
>     void showTextDialog(BuildContext context, String title, String content) {
>       showDialog(
>         context: context,
> @@ -232,8 +218,10 @@ class PVEWebConsoleState extends State<PVEWebConsole> {
>             value: ticket,
>           ),
>           builder: (context, snapshot) {
> -          return SafeArea(
> -            child: InAppWebView(
> +          return Scaffold(
> +            resizeToAvoidBottomInset: false,> +            appBar: PveAppBar(),
> +            body: InAppWebView(
>                 onReceivedServerTrustAuthRequest: (controller, challenge) async {
>                   final cert = challenge.protectionSpace.sslCertificate;
>                   final certBytes = cert?.x509Certificate?.encoded;
> @@ -292,8 +280,12 @@ class PVEWebConsoleState extends State<PVEWebConsole> {
>           });
>     }
>   
> -  Future<bool> showTLSWarning(BuildContext context, String sslError,
> -      String issuedTo, String hash) async {
> +  Future<bool> showTLSWarning(
> +    BuildContext context,
> +    String sslError,
> +    String issuedTo,
> +    String hash,
> +  ) async {
>       return await showDialog(
>           context: context,
>           barrierDismissible: false,

AFAICS this hunk only changes the indenation/formatting.
i guess your editor did sneak in some auto formatting ;)




_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel