Critical fixes based on code review

krystophny · krystophny · commit 9bc8cc6d4002 · 2025-10-23T15:47:43.000+02:00
Address all CRITICAL issues identified by codex review:

1. Fix PID retrieval: Use proper getpid() via C interop instead of non-existent $PID env var
2. Fix Windows support: Use platform-specific temp directories (%TEMP% vs /tmp)
3. Fix OS detection: Parse uname -s output to distinguish Darwin from Linux
4. Fix macOS display detection: Check SSH_CONNECTION and assume GUI available
5. Add temp file cleanup helpers (cleanup_temp_file subroutine)
6. Better error messages: Show temp file path when viewer fails
7. Use get_temp_filename() with PID + system_clock for uniqueness
8. Handle Windows path separators (backslash vs forward slash)

Remaining architecture concerns (filename==terminal, ASCII fallback) are
acceptable for initial implementation and can be improved in future PRs.
diff --git a/src/backends/raster/fortplot_png.f90 b/src/backends/raster/fortplot_png.f90
@@ -73,33 +73,26 @@ end function create_png_canvas
     ! All drawing methods are inherited from raster_context
 
     subroutine png_finalize(this, filename)
-        use fortplot_system_viewer, only: launch_system_viewer, has_graphical_session
+        use fortplot_system_viewer, only: launch_system_viewer, has_graphical_session, &
+                                          get_temp_filename
         class(png_context), intent(inout) :: this
         character(len=*), intent(in) :: filename
         character(len=1024) :: temp_file
         logical :: viewer_success
-        integer :: pid
 
         if (trim(filename) == 'terminal') then
             if (has_graphical_session()) then
-                call get_environment_variable('USER', temp_file)
-                if (len_trim(temp_file) == 0) temp_file = 'user'
-                call get_environment_variable('PID', temp_file)
-                if (len_trim(temp_file) == 0) then
-                    pid = 0
-                else
-                    read(temp_file, *) pid
-                end if
-                write(temp_file, '(A,I0,A)') '/tmp/fortplot_show_', pid, '.png'
-
+                call get_temp_filename('.png', temp_file)
                 call write_png_file(temp_file, this%width, this%height, this%raster%image_data)
                 call launch_system_viewer(temp_file, viewer_success)
                 if (.not. viewer_success) then
-                    call log_error("Failed to launch PNG viewer")
+                    call log_error("Failed to launch PNG viewer for: " // trim(temp_file))
+                    call log_info("You can manually open: " // trim(temp_file))
                 end if
             else
-                call log_info("No graphical session detected, falling back to ASCII")
-                call fallback_to_ascii(this)
+                call log_info("No graphical session detected, cannot display PNG")
+                call log_info("Use savefig('filename.png') to save to file or")
+                call log_info("Use savefig('filename.txt') for ASCII rendering")
             end if
         else
             call write_png_file(filename, this%width, this%height, this%raster%image_data)
diff --git a/src/backends/vector/fortplot_pdf.f90 b/src/backends/vector/fortplot_pdf.f90
@@ -172,28 +172,22 @@ subroutine draw_pdf_text_wrapper(this, x, y, text)
     end subroutine draw_pdf_text_wrapper
 
     subroutine write_pdf_file_facade(this, filename)
-        use fortplot_system_viewer, only: launch_system_viewer, has_graphical_session
+        use fortplot_system_viewer, only: launch_system_viewer, has_graphical_session, &
+                                          get_temp_filename
         class(pdf_context), intent(inout) :: this
         character(len=*), intent(in) :: filename
         logical :: file_success
-        character(len=1024) :: temp_file, actual_filename
+        character(len=1024) :: actual_filename
         logical :: viewer_success
-        integer :: pid
 
         ! Handle terminal display
         if (trim(filename) == 'terminal') then
             if (has_graphical_session()) then
-                call get_environment_variable('USER', temp_file)
-                if (len_trim(temp_file) == 0) temp_file = 'user'
-                call get_environment_variable('PID', temp_file)
-                if (len_trim(temp_file) == 0) then
-                    pid = 0
-                else
-                    read(temp_file, *) pid
-                end if
-                write(actual_filename, '(A,I0,A)') '/tmp/fortplot_show_', pid, '.pdf'
+                call get_temp_filename('.pdf', actual_filename)
             else
                 call log_info("No graphical session detected, cannot display PDF")
+                call log_info("Use savefig('filename.pdf') to save to file or")
+                call log_info("Use savefig('filename.txt') for ASCII rendering")
                 return
             end if
         else
@@ -229,7 +223,8 @@ subroutine write_pdf_file_facade(this, filename)
         if (trim(filename) == 'terminal' .and. has_graphical_session()) then
             call launch_system_viewer(actual_filename, viewer_success)
             if (.not. viewer_success) then
-                call log_error("Failed to launch PDF viewer")
+                call log_error("Failed to launch PDF viewer for: " // trim(actual_filename))
+                call log_info("You can manually open: " // trim(actual_filename))
             end if
         end if
     end subroutine write_pdf_file_facade
diff --git a/src/system/fortplot_system_viewer.f90 b/src/system/fortplot_system_viewer.f90
@@ -2,22 +2,45 @@ module fortplot_system_viewer
     !! System viewer launching for show() functionality
     !! Handles platform-specific viewer launching and graphical session detection
     use, intrinsic :: iso_fortran_env, only: wp => real64
+    use, intrinsic :: iso_c_binding, only: c_int
     use fortplot_logging, only: log_info, log_error
     implicit none
     private
 
     public :: launch_system_viewer
     public :: has_graphical_session
+    public :: get_temp_filename
+    public :: cleanup_temp_file
+
+    interface
+        function c_getpid() bind(C, name="getpid") result(pid)
+            import :: c_int
+            integer(c_int) :: pid
+        end function c_getpid
+    end interface
 
 contains
 
     function has_graphical_session() result(has_display)
         !! Check if a graphical session is available
-        !! Returns .true. if X11, Wayland, or other display is available
+        !! Returns .true. if X11, Wayland, macOS GUI, or Windows GUI available
         logical :: has_display
-        character(len=256) :: display_var, wayland_var, session_type
+        character(len=256) :: display_var, wayland_var, session_type, ssh_var
+        character(len=32) :: os_type
 
         has_display = .false.
+        os_type = get_os_type()
+
+        if (trim(os_type) == 'windows') then
+            has_display = .true.
+            return
+        end if
+
+        if (trim(os_type) == 'darwin') then
+            call get_environment_variable('SSH_CONNECTION', ssh_var)
+            has_display = len_trim(ssh_var) == 0
+            return
+        end if
 
         call get_environment_variable('DISPLAY', display_var)
         if (len_trim(display_var) > 0) then
@@ -89,37 +112,107 @@ end function get_viewer_command
     function get_os_type() result(os_type)
         !! Detect operating system type
         character(len=32) :: os_type
-        character(len=256) :: ostype_var
-        integer :: stat
+        character(len=256) :: ostype_var, uname_s
+        integer :: stat, uname_unit, ios
+
+        call get_environment_variable('OS', ostype_var)
+        if (index(ostype_var, 'Windows') > 0) then
+            os_type = 'windows'
+            return
+        end if
+
+        call execute_command_line('uname -s > /tmp/fortplot_uname.txt', exitstat=stat)
+        if (stat == 0) then
+            open(newunit=uname_unit, file='/tmp/fortplot_uname.txt', status='old', iostat=ios)
+            if (ios == 0) then
+                read(uname_unit, '(A)', iostat=ios) uname_s
+                close(uname_unit, status='delete')
+                if (index(uname_s, 'Darwin') > 0) then
+                    os_type = 'darwin'
+                    return
+                else if (index(uname_s, 'Linux') > 0) then
+                    os_type = 'linux'
+                    return
+                end if
+            end if
+        end if
 
         call get_environment_variable('OSTYPE', ostype_var)
-        if (len_trim(ostype_var) > 0) then
-            if (index(ostype_var, 'linux') > 0) then
-                os_type = 'linux'
-                return
-            else if (index(ostype_var, 'darwin') > 0) then
-                os_type = 'darwin'
+        if (index(ostype_var, 'darwin') > 0 .or. index(ostype_var, 'Darwin') > 0) then
+            os_type = 'darwin'
+        else if (index(ostype_var, 'linux') > 0 .or. index(ostype_var, 'Linux') > 0) then
+            os_type = 'linux'
+        else if (index(ostype_var, 'win') > 0 .or. index(ostype_var, 'msys') > 0) then
+            os_type = 'windows'
+        else
+            os_type = 'linux'
+        end if
+    end function get_os_type
+
+    function get_temp_dir() result(tempdir)
+        !! Get platform-specific temporary directory
+        character(len=256) :: tempdir
+        character(len=256) :: temp_env
+        character(len=32) :: os_type
+
+        os_type = get_os_type()
+
+        if (trim(os_type) == 'windows') then
+            call get_environment_variable('TEMP', temp_env)
+            if (len_trim(temp_env) > 0) then
+                tempdir = trim(temp_env)
                 return
-            else if (index(ostype_var, 'win') > 0 .or. index(ostype_var, 'msys') > 0) then
-                os_type = 'windows'
+            end if
+            call get_environment_variable('TMP', temp_env)
+            if (len_trim(temp_env) > 0) then
+                tempdir = trim(temp_env)
                 return
             end if
+            tempdir = 'C:\Temp'
+        else
+            tempdir = '/tmp'
         end if
+    end function get_temp_dir
+
+    subroutine get_temp_filename(extension, filename)
+        !! Generate unique temporary filename with proper platform handling
+        character(len=*), intent(in) :: extension
+        character(len=*), intent(out) :: filename
+        character(len=256) :: tempdir
+        character(len=1) :: sep
+        character(len=32) :: os_type
+        integer(c_int) :: pid
+        integer :: clk_count, clk_rate, clk_max
 
-        call execute_command_line('uname', exitstat=stat)
-        if (stat == 0) then
-            os_type = 'linux'
-            return
-        end if
+        tempdir = get_temp_dir()
+        os_type = get_os_type()
+        pid = c_getpid()
+        call system_clock(clk_count, clk_rate, clk_max)
 
-        call get_environment_variable('OS', ostype_var)
-        if (index(ostype_var, 'Windows') > 0) then
-            os_type = 'windows'
-            return
+        if (trim(os_type) == 'windows') then
+            sep = '\'
+        else
+            sep = '/'
         end if
 
-        os_type = 'linux'
-    end function get_os_type
+        write(filename, '(A,A,A,I0,A,I0,A)') trim(tempdir), sep, 'fortplot_show_', &
+              int(pid), '_', clk_count, trim(extension)
+    end subroutine get_temp_filename
+
+    subroutine cleanup_temp_file(filename)
+        !! Remove temporary file (best effort, no error on failure)
+        character(len=*), intent(in) :: filename
+        integer :: stat
+        logical :: exists
+
+        inquire(file=trim(filename), exist=exists)
+        if (exists) then
+            open(newunit=stat, file=trim(filename), status='old', iostat=stat)
+            if (stat == 0) then
+                close(stat, status='delete', iostat=stat)
+            end if
+        end if
+    end subroutine cleanup_temp_file
 
     function int_to_str(i) result(str)
         !! Convert integer to string
diff --git a/test_show.f90 b/test_show.f90
@@ -0,0 +1,35 @@
+program test_show
+    use fortplot_figure, only: figure_t
+    implicit none
+
+    type(figure_t) :: fig
+    real :: x(100), y(100)
+    integer :: i
+
+    ! Generate simple test data
+    do i = 1, 100
+        x(i) = real(i) / 10.0
+        y(i) = sin(x(i))
+    end do
+
+    ! Test PNG backend with terminal display
+    print *, "Testing PNG backend show()..."
+    call fig%init(backend='PNG')
+    call fig%plot(x, y)
+    call fig%show()  ! Should launch viewer or log message
+
+    ! Test PDF backend with terminal display
+    print *, "Testing PDF backend show()..."
+    call fig%init(backend='PDF')
+    call fig%plot(x, y)
+    call fig%show()  ! Should launch viewer or log message
+
+    ! Test ASCII backend with terminal display
+    print *, "Testing ASCII backend show()..."
+    call fig%init(backend='ASCII')
+    call fig%plot(x, y)
+    call fig%show()  ! Should output to terminal
+
+    print *, "Tests complete"
+
+end program test_show
