Review Request 127706: adaptations for OS X

[Emmanuel Pescosta]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127706/#review94949
-----------------------------------------------------------



Thanks for the patch! Adaptations for OS X and Windows are welcome of course ;)

Sorry for the late reply, missed the email from RR somehow ...


src/dolphinmainwindow.cpp (lines 170 - 177)
<https://git.reviewboard.kde.org/r/127706/#comment64459>

    Wouldn't it be better to just disable the ShowMenubar action and maybe force menuBar()->setVisible(true) on OS X?
    
    This would reduce the number of required changes and avoid that showMenu is still toggle-able.
    
    What do you think?


- Emmanuel Pescosta


On April 22, 2016, 2:40 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127706/
> -----------------------------------------------------------
> 
> (Updated April 22, 2016, 2:40 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> Is there any incentive to support Dolphin on OS X (and MS Windows)?
> 
> If so, this patch introduces 2 changes:
> 
> - the application icon isn't replaced unconditionally by the result of QIcon::fromTheme() but instead that call receives the current application icon as a fallback. This avoids removing the existing application icon in cases where icon themes are not functional (default on OS X and MS Windows).
> 
> - deactivates the control menu button in the toolbar in case the menubar is always present (= on OS X). This prevents continuous error messages about menu items being reassigned, due to reparenting QActionWidgets (= using a single instance in multiple menus). It also avoids a crash-on-exit that seems to be related to QActionWidget reuse (double free somewhere deep Qt).
> 
> As to adding an application icon on OS X (and presumably MS Windows): this seems no longer possible from an installed icon theme but apparently requires shipping the icons nowadays. Using an installed icon theme requires an additional step to set up properly named symlinks (in the build directory; icon file names are required to contain their resolution in their name). Once that is done, the following patch takes care of the rest:
> 
> ```
> --- src/orig.CMakeLists.txt	2016-04-13 18:49:42.000000000 +0200
> +++ src/CMakeLists.txt	2016-04-21 17:08:56.000000000 +0200
> @@ -1,3 +1,4 @@
> +include(ECMAddAppIcon)
>  
>  configure_file(config-baloo.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-baloo.h)
>  
> @@ -270,6 +271,10 @@
>  # TODO Does anything replace kde4_add_app_icon ?
>  #kde4_add_app_icon(dolphin_SRCS "${KDE4_ICON_INSTALL_DIR}/oxygen/*/apps/system-file-manager.png")
>  
> +# Sets the icon on Windows and OSX
> +file(GLOB ICONS_SRCS "${CMAKE_CURRENT_BINARY_DIR}/icons/*system-file-manager.png")
> +ecm_add_app_icon(dolphin_SRCS ICONS ${ICONS_SRCS})
> +
>  kf5_add_kdeinit_executable(dolphin ${dolphin_SRCS})
>  
>  target_include_directories(kdeinit_dolphin PRIVATE ${PHONON_INCLUDES})
> ```
> 
> 
> Diffs
> -----
> 
>   src/dolphinmainwindow.cpp d4f2b06 
>   src/main.cpp 0bbae97 
> 
> Diff: https://git.reviewboard.kde.org/r/127706/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.9.5 with Qt 5.6.0, FWs 5.20.0 and dependencies installed in /opt/local
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20160428/4642908e/attachment.htm>