bug#45620: 28.0.50; Child frames should have their own border width and colour

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alexander Miller]

This is an idea set off by this discussion:
https://github.com/Alexander-Miller/treemacs/issues/242#issuecomment-753296634

Basically the problem is that child frames need a distinct border colour
to be properly visible (and just using a different background colour does
not look nearly as good).

That can be achieved by customising the `internal-border' face, however
that face controls the appearance of the border of all frames, and as I
have learned there is a non-trivial amount of users who by default use a
large internal border as a margin for their frames. And for these users
the `internal-border' should have the same colour as the default
background.

So there is a conflict between wanting a distinct border colour for
child frames and using the default background for normal frames.

Packages can work around that individually, for example posframe accepts
an `internal-border-color' argument that overrides the `internal-border'
face frame-locally. But that still means that every package using child
frames needs its own user option for the child frames' border colour
when such matters clearly belong under the domain of the user's theme.

I think that since child frames serve sufficiently distinct use cases
than normal frames it makes sense for them to have their own border
appearance controls.

In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.23, cairo version 1.17.3)
of 2020-12-20 built on am-laptop
Repository revision: ab985f41db5fdaeada513d28a065332fd8838cf4
Repository branch: makepkg
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Manjaro Linux

Configured using:
'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
--localstatedir=/var --mandir=/usr/share/man --with-gameuser=:games
--with-sound=alsa --with-modules --without-gconf --without-gsettings
--with-nativecomp --with-x-toolkit=gtk3 --without-xaw3d
--without-m17n-flt --with-cairo --without-compress-install
'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -g
-fuse-ld=gold' CPPFLAGS=-D_FORTIFY_SOURCE=2
LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GLIB NOTIFY INOTIFY ACL
GNUTLS LIBXML2 FREETYPE HARFBUZZ LIBOTF ZLIB TOOLKIT_SCROLL_BARS GTK3
X11 XDBE XIM MODULES NATIVE_COMP THREADS LIBSYSTEMD JSON PDUMPER LCMS2

Important settings:
value of $LC_MONETARY: de_DE.UTF-8
value of $LC_NUMERIC: de_DE.UTF-8
value of $LC_TIME: de_DE.UTF-8
value of $LANG: en_GB.utf8
locale-coding-system: utf-8

Major mode: Fundamental

Minor modes in effect:
helm-mode: t
helm--remap-mouse-mode: t
async-bytecomp-package-mode: t
projectile-mode: t
company-prescient-mode: t
global-company-mode: t
company-mode: t
global-evil-vimish-fold-mode: t
show-smartparens-global-mode: t
show-smartparens-mode: t
smartparens-global-mode: t
smartparens-mode: t
org-super-agenda-mode: t
treemacs-icons-dired-mode: t
treemacs-filewatch-mode: t
treemacs-follow-mode: t
treemacs-git-mode: deferred
treemacs-fringe-indicator-mode: t
gcmh-mode: t
winner-mode: t
framey-mode: t
purpose-mode: t
shackle-mode: t
winum-mode: t
eyebrowse-mode: t
global-ligature-mode: t
ligature-mode: t
global-subword-mode: t
subword-mode: t
global-evil-surround-mode: t
evil-surround-mode: t
evil-lion-mode: t
evil-goggles-mode: t
shell-dirtrack-mode: t
evil-mode: t
evil-local-mode: t
tooltip-mode: t
global-eldoc-mode: t
electric-indent-mode: t
mouse-wheel-mode: t
tool-bar-mode: t
menu-bar-mode: t
file-name-shadow-mode: t
global-font-lock-mode: t
font-lock-mode: t
auto-composition-mode: t
auto-encryption-mode: t
auto-compression-mode: t
line-number-mode: t
transient-mark-mode: t

Load-path shadows:
~/Documents/git/cfrs/cfrs hides /home/am/.emacs.d/straight/build/cfrs/cfrs
/usr/share/emacs/28.0.50/lisp/emacs-lisp/let-alist hides
/home/am/.emacs.d/straight/build/let-alist/let-alist
~/Documents/git/treemacs/src/elisp/treemacs-interface hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-interface
~/Documents/git/treemacs/src/elisp/treemacs-rendering hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-rendering
~/Documents/git/treemacs/src/elisp/treemacs-hydras hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-hydras
~/Documents/git/treemacs/src/elisp/treemacs-dom hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-dom
~/Documents/git/treemacs/src/elisp/treemacs-tags hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-tags
~/Documents/git/treemacs/src/elisp/treemacs hides
/home/am/.emacs.d/straight/build/treemacs/treemacs
~/Documents/git/treemacs/src/elisp/treemacs-workspaces hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-workspaces
~/Documents/git/treemacs/src/elisp/treemacs-customization hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-customization
~/Documents/git/treemacs/src/elisp/treemacs-faces hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-faces
~/Documents/git/treemacs/src/elisp/treemacs-themes hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-themes
~/Documents/git/treemacs/src/elisp/treemacs-mouse-interface hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-mouse-interface
~/Documents/git/treemacs/src/elisp/treemacs-mode hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-mode
~/Documents/git/treemacs/src/elisp/treemacs-icons hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-icons
~/Documents/git/treemacs/src/elisp/treemacs-compatibility hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-compatibility
~/Documents/git/treemacs/src/elisp/treemacs-follow-mode hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-follow-mode
~/Documents/git/treemacs/src/elisp/treemacs-visuals hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-visuals
~/Documents/git/treemacs/src/elisp/treemacs-core-utils hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-core-utils
~/Documents/git/treemacs/src/elisp/treemacs-extensions hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-extensions
~/Documents/git/treemacs/src/elisp/treemacs-filewatch-mode hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-filewatch-mode
~/Documents/git/treemacs/src/elisp/treemacs-persistence hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-persistence
~/Documents/git/treemacs/src/elisp/treemacs-async hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-async
~/Documents/git/treemacs/src/elisp/treemacs-bookmarks hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-bookmarks
~/Documents/git/treemacs/src/elisp/treemacs-tag-follow-mode hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-tag-follow-mode
~/Documents/git/treemacs/src/elisp/treemacs-logging hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-logging
~/Documents/git/treemacs/src/elisp/treemacs-header-line hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-header-line
~/Documents/git/treemacs/src/elisp/treemacs-fringe-indicator hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-fringe-indicator
~/Documents/git/treemacs/src/elisp/treemacs-diagnostics hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-diagnostics
~/Documents/git/treemacs/src/elisp/treemacs-macros hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-macros
~/Documents/git/treemacs/src/elisp/treemacs-scope hides
/home/am/.emacs.d/straight/build/treemacs/treemacs-scope

Features:
(shadow ispell rainbow-delimiters rainbow-mode xterm-color
display-line-numbers hl-todo prose-complete emacsbug face-remap
helm-command helm-elisp helm-eval edebug backtrace helm-info info sort
gnus-cite smiley shr-color color mm-archive mail-extr gnus-art mm-uu
mml2015 mm-view mml-smime smime dig gnus-sum shr kinsoku svg dom
gnus-group gnus-undo gnus-start gnus-dbus gnus-cloud nnimap nnmail
mail-source utf7 netrc nnoo gnus-spec gnus-int gnus-range gnus-win gnus
nnheader wid-edit pp view helm-mode helm-projectile helm-files
docker-tramp tramp-cache tramp tramp-loaddefs trampver tramp-integration
files-x tramp-compat parse-time iso8601 ls-lisp helm-tags helm-buffers
helm-occur helm-grep helm-regexp helm-utils helm-locate helm-help
helm-types framey-helm helm-config helm async-bytecomp
helm-global-bindings helm-source helm-multi-match helm-lib async
treemacs-projectile projectile grep compile ibuf-ext ibuffer
ibuffer-loaddefs mu4e-alert time alert log4e notifications dbus
company-keywords company-dabbrev-code company-dabbrev company-yasnippet
company-files company-capf company-prescient prescient company server
evil-vimish-fold vimish-fold smartparens-config smartparens-org
smartparens-text paren smartparens xml gntp org-mu4e mu4e desktop
frameset mu4e-org german-holidays org-super-agenda ts org-habit
org-element avl-tree generator org-agenda org-refile org ob ob-tangle
ob-ref ob-lob ob-table ob-exp org-macro org-footnote org-src ob-comint
org-pcomplete org-list org-faces org-entities noutline outline
org-version ob-emacs-lisp ob-core ob-eval org-table ol org-keys
org-compat org-macs org-loaddefs find-func mu4e-main mu4e-view cal-menu
calendar cal-loaddefs browse-url url url-proxy url-privacy url-expand
url-methods url-history url-cookie url-domsuf url-util url-parse
url-vars mailcap mu4e-headers mu4e-compose mu4e-context mu4e-draft
mu4e-actions ido rfc2368 smtpmail sendmail mu4e-mark mu4e-message
flow-fill mu4e-proc mu4e-utils doc-view jka-compr mu4e-lists mule-util
mu4e-vars message rmc puny treemacs-icons-dired treemacs-evil treemacs
treemacs-header-line treemacs-compatibility treemacs-mode
treemacs-interface treemacs-extensions treemacs-mouse-interface
treemacs-tags imenu xref project treemacs-persistence
treemacs-filewatch-mode filenotify treemacs-follow-mode
treemacs-rendering treemacs-async treemacs-workspaces treemacs-dom
treemacs-visuals treemacs-fringe-indicator treemacs-scope treemacs-faces
treemacs-icons treemacs-themes treemacs-core-utils pfuture hl-line
treemacs-logging treemacs-customization treemacs-macros dired+
image-dired image-mode exif image-file image-converter dired-x dired-aux
dired dired-loaddefs rfc822 mml mml-sec epa derived epg epg-config
gnus-util rmail rmail-loaddefs auth-source password-cache json map
text-property-search time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr mailabbrev
mail-utils gmm-utils mailheader mu4e-meta gcmh doom-modeline
doom-modeline-segments doom-modeline-env doom-modeline-core shrink-path
f all-the-icons all-the-icons-faces data-material data-weathericons
data-octicons data-fileicons data-faicons data-alltheicons memoize
winner framey inline ht s window-purpose window-purpose-fixes
window-purpose-prefix-overload window-purpose-switch let-alist
window-purpose-layout window-purpose-core window-purpose-configuration
eieio-compat eieio window-purpose-utils shackle trace winum eyebrowse
format-spec ligature morning-star-theme cap-words superword subword
evil-surround evil-lion evil-goggles pulse evil evil-keybindings
evil-integration evil-maps evil-commands reveal evil-jumps
evil-command-window evil-types evil-search evil-ex shell pcomplete
comint ansi-color evil-macros evil-repeat evil-states evil-core comp
comp-cstr warnings subr-x cl-extra help-mode seq byte-opt bytecomp
byte-compile cconv advice evil-common windmove thingatpt rect
evil-digraphs evil-vars ring edmacro kmacro cl-seq dash
yequake-autoloads yasnippet-autoloads yaml-mode-autoloads
xwidgets-reuse-autoloads xterm-color-autoloads wttrin-autoloads
writeroom-mode-autoloads with-editor-autoloads winum-autoloads
window-purpose-autoloads eieio-core cl-macs eieio-loaddefs
vterm-autoloads visual-fill-column-autoloads vimish-fold-autoloads
ts-autoloads tridactylrc-mode-autoloads treepy-autoloads
treemacs-autoloads transient-posframe-autoloads transient-autoloads
toml-mode-autoloads toc-org-autoloads tablist-autoloads
straight-autoloads spinner-autoloads smartparens-autoloads
shrink-path-autoloads shackle-autoloads s-autoloads rust-mode-autoloads
restart-emacs-autoloads rainbow-mode-autoloads
rainbow-delimiters-autoloads projectile-autoloads pretty-hydra-autoloads
prescient-autoloads posframe-autoloads pos-tip-autoloads popup-autoloads
pkg-info-autoloads pfuture-autoloads perspective-autoloads
persp-mode-autoloads peep-dired-autoloads pdf-tools-autoloads
pcre2el-autoloads package-lint-autoloads org-superstar-autoloads
org-super-agenda-autoloads org-autoloads multi-compile-autoloads
mu4e-views-autoloads mu4e-alert-autoloads morning-star-autoloads
memoize-autoloads markdown-mode-autoloads magit-todos-autoloads
magit-autoloads macrostep-autoloads lv-autoloads lsp-ui-autoloads
lsp-treemacs-autoloads lsp-mode-autoloads log4e-autoloads
link-hint-autoloads ligature-autoloads let-alist-autoloads
ledger-mode-autoloads json-snatcher-autoloads json-reformat-autoloads
json-mode-autoloads rx imenu-list-autoloads i3wm-config-mode-autoloads
hydra-autoloads ht-autoloads hl-todo-autoloads helpful-autoloads
helm-projectile-autoloads helm-org-autoloads helm-easymenu easymenu
helm-core-autoloads helm-ag-autoloads helm-autoloads goto-chg-autoloads
gntp-autoloads git-modes-autoloads git-gutter-fringe-autoloads
git-gutter-autoloads git-commit-autoloads ghub-autoloads
german-holidays-autoloads gcmh-autoloads frog-menu-autoloads
fringe-helper-autoloads framey-autoloads frame-local-autoloads
forge-autoloads flyspell-correct-autoloads flycheck-pos-tip-autoloads
flycheck-autoloads fish-mode-autoloads fill-column-indicator-autoloads
f-autoloads eyebrowse-autoloads expand-region-autoloads
evil-vimish-fold-autoloads evil-surround-autoloads
evil-numbers-autoloads evil-nerd-commenter-autoloads
evil-magit-autoloads evil-lion-autoloads evil-ledger-autoloads
evil-goggles-autoloads evil-exchange-autoloads evil-collection-autoloads
evil-autoloads eros-autoloads epl-autoloads epkg-autoloads
emacsql-sqlite-autoloads emacsql-autoloads elisp-refs-autoloads
elfeed-org-autoloads elfeed-autoloads doom-modeline-autoloads
doct-autoloads dockerfile-mode-autoloads docker-tramp-autoloads
docker-compose-mode-autoloads docker-autoloads dired+-autoloads
dash-functional-autoloads dash-autoloads ctrlf-autoloads easy-mmode
company-shell-autoloads company-quickhelp-autoloads
company-prescient-autoloads company-box-autoloads company-autoloads
closql-autoloads cfrs-autoloads buttercup-autoloads avy-autoloads
async-autoloads anzu-autoloads annalist-autoloads
all-the-icons-autoloads alert-autoloads ace-window-autoloads cl-loaddefs
cl-lib gv iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face pcase macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting font-render-setting cairo move-toolbar gtk
x-toolkit x multi-tty make-network-process nativecomp emacs)

Memory information:
((conses 16 668840 513173)
(symbols 48 43798 0)
(strings 32 152762 59495)
(string-bytes 1 5475171)
(vectors 16 63689)
(vector-slots 8 1094531 320683)
(floats 8 926 1405)
(intervals 56 6773 8846)
(buffers 984 21))

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 > This is an idea set off by this discussion:
 > https://github.com/Alexander-Miller/treemacs/issues/242#issuecomment-753296634
 >
 > Basically the problem is that child frames need a distinct border colour
 > to be properly visible (and just using a different background colour does
 > not look nearly as good).
 >
 > That can be achieved by customising the `internal-border' face, however
 > that face controls the appearance of the border of all frames, and as I
 > have learned there is a non-trivial amount of users who by default use a
 > large internal border as a margin for their frames. And for these users
 > the `internal-border' should have the same colour as the default
 > background.

Isn't the situation even worse than how you describe it here?  When I
customize 'internal-border' face, that affects all frames, including
those for which I have set it already via 'set-face-background'.  Which
means that whatever a package does to set that face for a specific
(child) frame, that setting is undone by a later customization.  IIUC
the discussion you refer to above arrived at the same conclusion.

 > So there is a conflict between wanting a distinct border colour for
 > child frames and using the default background for normal frames.

If I'm not mistaken we use that face for our tooltip frames too which
means one more conflict.

 > Packages can work around that individually, for example posframe accepts
 > an `internal-border-color' argument that overrides the `internal-border'
 > face frame-locally. But that still means that every package using child
 > frames needs its own user option for the child frames' border colour
 > when such matters clearly belong under the domain of the user's theme.

"clearly" is clearly too strong here.  Ultimately, the package must have
the choice and its choice should prevail (it currently doesn't).

 > I think that since child frames serve sufficiently distinct use cases
 > than normal frames it makes sense for them to have their own border
 > appearance controls.

So what should we do?  Provide a separate 'child-frame-internal-border'
face and then probably also a 'tooltip-internal-border-face'?

Customizing such a face would still override individual frame settings.
What we need is probably a strategy to avoid setting the background for
those frames that have their internal border already set.  But then we
should do that for all faces running through 'set-face-attribute'.

martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alexander Miller]

 > Isn't the situation even worse than how you describe it here? When I
 > customize 'internal-border' face, that affects all frames, including
 > those for which I have set it already via 'set-face-background'. Which
 > means that whatever a package does to set that face for a specific
 > (child) frame, that setting is undone by a later customization. IIUC
 > the discussion you refer to above arrived at the same conclusion.

What customisations are you referring to? I cannot think of any
scenario, other than changing and reloading your theme, that could
change the settings of already present child frames.

 > If I'm not mistaken we use that face for our tooltip frames too which
 > means one more conflict.

Partially. On my system the internal-border colour and width only
applied to the top and left sides of the tooltip frame.

 > "clearly" is clearly too strong here. Ultimately, the package must have
 > the choice and its choice should prevail (it currently doesn't).

Packages do have the choice, they just have to make sure to override the
frame-local faces whenever they show something. The problem, as I see
it, is that they *have to* create a face to override the local
internal-border, instead of just having the option to do it, because
themes cannot offer a general setting, like an easily visible dark
border on a bright foreground, because they run into the frame margin
colour conflict like modus did in the linked issue. So if a package
doesn't overwrite the local internal-border your default look-and-feel
is a badly visible same-colour-on-same-colour popup.

 > So what should we do? Provide a separate 'child-frame-internal-border'
 > face and then probably also a 'tooltip-internal-border-face'?

That would be perfectly good enough for themes like modus, yes.

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 > What customisations are you referring to? I cannot think of any
 > scenario, other than changing and reloading your theme, that could
 > change the settings of already present child frames.

Here I'm using a child frame for popping up the minibuffer with the
customization

(defcustom pop-up-mini-internal-border "blue"
   "Background of internal border of 'pop-up-mini-frame'."
   ...

and the form

   (set-face-background
    'internal-border pop-up-mini-internal-border pop-up-mini-frame)

to put it into practice when setting up such a frame.  This works fine
until I do M-x customize-face RET internal-border RET.  As soon as I
apply the value I customized there, my minibuffer child frame gets that
border instead of the one specified via 'pop-up-mini-internal-border'
above.

 >  > If I'm not mistaken we use that face for our tooltip frames too which
 >  > means one more conflict.
 >
 > Partially. On my system the internal-border colour and width only
 > applied to the top and left sides of the tooltip frame.

Yes.  I think there's a bug somewhere but the only time I tried to
locate it, I decided that someone else intentionally does it that way.

 > Packages do have the choice, they just have to make sure to override the
 > frame-local faces whenever they show something. The problem, as I see
 > it, is that they *have to* create a face to override the local
 > internal-border, instead of just having the option to do it, because
 > themes cannot offer a general setting, like an easily visible dark
 > border on a bright foreground, because they run into the frame margin
 > colour conflict like modus did in the linked issue. So if a package
 > doesn't overwrite the local internal-border your default look-and-feel
 > is a badly visible same-colour-on-same-colour popup.

Agreed.  On a historical note, I had to add the entire internal border
entertainment to child frames because neither gtk nor X bother to supply
the normal window manager border for them (Windows does) and so you can
neither make them stand apart easily nor can you resize them with the
mouse.  Implementing the latter from within Emacs was even very
entertaining.  I intentionally left the border color alone because I'm
convinced that customizing it is a bad idea as sketched in the above
scenario and packages should always try their own way to specify it.

 >  > So what should we do? Provide a separate 'child-frame-internal-border'
 >  > face and then probably also a 'tooltip-internal-border-face'?
 >
 > That would be perfectly good enough for themes like modus, yes.

Can you propose a patch?  Look at where we use INTERNAL_BORDER_FACE_ID
to set up a face (x_clear_under_internal_border in xterm.c is an
example), check f for child-frameness there and use the new face if the
frame is a child frame.  But be aware: Users like me will then have to
separately set the background for the child frame face so you might get
incompatibility complaints ...

martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alexander Miller]

 > As soon as I apply the value I customized there, my minibuffer child
 > frame gets that border instead of the one specified via
 > 'pop-up-mini-internal-border' above.

Personally I would think this behaviour is mildly inconvenient at worst,
but hardly a real problem. It's just a new global default overriding
local customisations.

And that is only if I manage to run into the situation in the first
place - I am not usually in the habit of customising frame border faces,
and I don't think many other people are, either. So from my perspective
I don't really see the danger (or annoyance) in this behaviour.

 > Can you propose a patch?

I can *try*. I am absolutely not a C programmer, but as long as the task
is limited to a monkey see, monkey do situation for handling a new face
I should be able to hammer something useful together.

In fact my first attempt seems to compile and behave as expected, so I
have a few questions on how to proceed:

- I need to repeatedly use a pattern that looks like this:

int is_child_frame = FRAME_PARENT_FRAME(f) != NULL;
int border_face_id = is_child_frame
    ? CHILD_FRAME_BORDER_FACE_ID
    : INTERNAL_BORDER_FACE_ID;
int face_id = !NILP (Vface_remapping_alist)
    ? lookup_basic_face (NULL, f, border_face_id)
    : border_face_id;

and I would like to put it into a single function accessible from
anywhere. Is that the right call, and if yes, where would be the right
place to put it?

- Currently the actual width of the border is still controlled by the
`internal-border-width` parameter for both frame types. Should I try to
do something about that as well? If yes, what's my entry point?

- I think I'll need to sign the FSF copyright assignment, unless the
limit is higher than the 15 lines I am remembering.

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 >  > Can you propose a patch?
 >
 > I can *try*. I am absolutely not a C programmer, but as long as the task
 > is limited to a monkey see, monkey do situation for handling a new face
 > I should be able to hammer something useful together.

That's one way to become a C programmer ...

 > In fact my first attempt seems to compile and behave as expected, so I
 > have a few questions on how to proceed:
 >
 > - I need to repeatedly use a pattern that looks like this:
 >
 > int is_child_frame = FRAME_PARENT_FRAME(f) != NULL;

In that case you should make is_child_frame a bool and not an int.  But
it's much simpler to just test for FRAME_PARENT_FRAME (f) wherever you
see a need for is_child_frame like in

 > int border_face_id = is_child_frame

   int border_face_id = FRAME_PARENT_FRAME (f)

 >     ? CHILD_FRAME_BORDER_FACE_ID
 >     : INTERNAL_BORDER_FACE_ID;
 > int face_id = !NILP (Vface_remapping_alist)
 >     ? lookup_basic_face (NULL, f, border_face_id)
 >     : border_face_id;
 >
 > and I would like to put it into a single function accessible from
 > anywhere. Is that the right call, and if yes, where would be the right
 > place to put it?

This is the first time I see the internal border face getting remapped.
I wasn't aware that nsterm.c does that and I'm not sure whether we
should add something similar to xterm.c and w32term.c.  In nsterm.c I
would not write an extra function but instead of what we have now use

       int face_id =
	(FRAME_PARENT_FRAME (f)
	 ? (!NILP (Vface_remapping_alist)
	    ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
	    : CHILD_FRAME_BORDER_FACE_ID)
	 : (!NILP (Vface_remapping_alist)
	    ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
	    : INTERNAL_BORDER_FACE_ID));

and in lookup_basic_face in xfaces.c you'd then have to add

     case CHILD_FRAME_BORDER_FACE_ID:	name = Qchild_frame_border; 	break;

It's augmenting forms like that last one that get the most obscure bugs,
so don't despair.

 > - Currently the actual width of the border is still controlled by the
 > `internal-border-width` parameter for both frame types. Should I try to
 > do something about that as well? If yes, what's my entry point?

Add a 'child-frame-border-width' parameter.  But in this case I would
propose to proceed as follows:

- If for a frame the 'child-frame-border-width' was explicitly set, use
   it.

- If it was not set, use the 'internal-border-width' parameter.

Note that people have already set up their own child frame creation
routines and we should try to not punish them too much.  And please try
to discuss this on the list you cited earlier and also with the
posframe.el designer.  Mister Feng Shu you've inevitably become a
participant of this thread now.

 > - I think I'll need to sign the FSF copyright assignment, unless the
 > limit is higher than the 15 lines I am remembering.

I think so too.

martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alexander Miller]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

 >>> Can you propose a patch?
 >>
 >> I can *try*. I am absolutely not a C programmer, but as long as the
task
 >> is limited to a monkey see, monkey do situation for handling a new face
 >> I should be able to hammer something useful together.
 >
 > That's one way to become a C programmer ...

To be honest I've don't really like C as a language, and learning it
would not do anything professionally for me either. So I'm jumping into
this strictly for the sake of improving Emacs.

 >> and I would like to put it into a single function accessible from
 >> anywhere. Is that the right call, and if yes, where would be the right
 >> place to put it?
 >
 > This is the first time I see the internal border face getting remapped.
 > I wasn't aware that nsterm.c does that and I'm not sure whether we
 > should add something similar to xterm.c and w32term.c. In nsterm.c I
 > would not write an extra function but instead of what we have now use
 >
 > int face_id =
 > (FRAME_PARENT_FRAME (f)
 > ? (!NILP (Vface_remapping_alist)
 > ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
 > : CHILD_FRAME_BORDER_FACE_ID)
 > : (!NILP (Vface_remapping_alist)
 > ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
 > : INTERNAL_BORDER_FACE_ID));

x, ns and w32 all used the same code, so I changed them all to look like
this now.

 >> - Currently the actual width of the border is still controlled by the
 >> `internal-border-width` parameter for both frame types. Should I try to
 >> do something about that as well? If yes, what's my entry point?
 >
 > Add a 'child-frame-border-width' parameter. But in this case I would
 > propose to proceed as follows:
 >
 > - If for a frame the 'child-frame-border-width' was explicitly set, use
 > it.
 >
 > - If it was not set, use the 'internal-border-width' parameter.

That's done now too, at least for X. I'll attach patches of my first
working drafts for both changes. Let me know if I'm on the right path so
far.

 >> - I think I'll need to sign the FSF copyright assignment, unless the
 >> limit is higher than the 15 lines I am remembering.
 >
 > I think so too.
 >

Ok, what do I do?


[-- Attachment #2: 0001-WIP-child-frame-border.patch --]
[-- Type: text/x-patch, Size: 6405 bytes --]

From 50e3d31abf4bd12e24f82bb2f8e39ec1c3c073e0 Mon Sep 17 00:00:00 2001
From: Alexander Miller <alexanderm@web.de>
Date: Tue, 5 Jan 2021 11:33:31 +0100
Subject: [PATCH 1/2] WIP child-frame-border

---
 lisp/faces.el    | 11 ++++++++++-
 src/dispextern.h |  1 +
 src/nsterm.m     | 10 +++++++---
 src/w32fns.c     | 10 +++++++---
 src/xfaces.c     |  3 +++
 src/xterm.c      | 20 ++++++++++++++------
 6 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 4e98338432..639dbeb09a 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2683,11 +2683,20 @@ window-divider-last-pixel

 (defface internal-border
     '((t nil))
-  "Basic face for the internal border."
+  "Basic face for the internal border.
+For the internal border of child frames see `child-frame-border'."
   :version "26.1"
   :group 'frames
   :group 'basic-faces)

+(defface child-frame-border
+  '((t nil))
+  "Basic face for the internal border of child frames.
+For the internal border of non-child frames see `internal-border'."
+  :version "28.1"
+  :group 'frames
+  :group 'basic-faces)
+
 (defface minibuffer-prompt
   '((((background dark)) :foreground "cyan")
     ;; Don't use blue because many users of the MS-DOS port customize
diff --git a/src/dispextern.h b/src/dispextern.h
index 3ad98b8344..f4e872644d 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1826,6 +1826,7 @@ #define FACE_UNIBYTE_P(FACE) ((FACE)->charset < 0)
   WINDOW_DIVIDER_FIRST_PIXEL_FACE_ID,
   WINDOW_DIVIDER_LAST_PIXEL_FACE_ID,
   INTERNAL_BORDER_FACE_ID,
+  CHILD_FRAME_BORDER_FACE_ID,
   TAB_BAR_FACE_ID,
   TAB_LINE_FACE_ID,
   BASIC_FACE_ID_SENTINEL
diff --git a/src/nsterm.m b/src/nsterm.m
index 2defb9e2ee..76f67d2531 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3029,9 +3029,13 @@ so some key presses (TAB) are swallowed by the system.  */
       NSRectEdge edge[] = {NSMinXEdge, NSMinYEdge, NSMaxXEdge, NSMaxYEdge};

       int face_id =
-	!NILP (Vface_remapping_alist)
-	? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
-	: INTERNAL_BORDER_FACE_ID;
+        (FRAME_PARENT_FRAME (f)
+         ? (!NILP (Vface_remapping_alist)
+            ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
+            : CHILD_FRAME_BORDER_FACE_ID)
+         : (!NILP (Vface_remapping_alist)
+            ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
+            : INTERNAL_BORDER_FACE_ID));
       struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);

       if (!face)
diff --git a/src/w32fns.c b/src/w32fns.c
index c1e18ff7fa..061886f0bc 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -1519,9 +1519,13 @@ w32_clear_under_internal_border (struct frame *f)
       int width = FRAME_PIXEL_WIDTH (f);
       int height = FRAME_PIXEL_HEIGHT (f);
       int face_id =
-	!NILP (Vface_remapping_alist)
-	? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
-	: INTERNAL_BORDER_FACE_ID;
+	(FRAME_PARENT_FRAME (f)
+	 ? (!NILP (Vface_remapping_alist)
+	    ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
+	    : CHILD_FRAME_BORDER_FACE_ID)
+	 : (!NILP (Vface_remapping_alist)
+	    ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
+	    : INTERNAL_BORDER_FACE_ID));
       struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);

       block_input ();
diff --git a/src/xfaces.c b/src/xfaces.c
index b3b19a9cb2..5080120957 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -4913,6 +4913,7 @@ lookup_basic_face (struct window *w, struct frame *f, int face_id)
     case WINDOW_DIVIDER_FIRST_PIXEL_FACE_ID:	name = Qwindow_divider_first_pixel;	break;
     case WINDOW_DIVIDER_LAST_PIXEL_FACE_ID:	name = Qwindow_divider_last_pixel;	break;
     case INTERNAL_BORDER_FACE_ID:	name = Qinternal_border; 	break;
+    case CHILD_FRAME_BORDER_FACE_ID:	name = Qchild_frame_border; 	break;

     default:
       emacs_abort (); /* the caller is supposed to pass us a basic face id */
@@ -5619,6 +5620,7 @@ realize_basic_faces (struct frame *f)
       realize_named_face (f, Qwindow_divider_last_pixel,
 			  WINDOW_DIVIDER_LAST_PIXEL_FACE_ID);
       realize_named_face (f, Qinternal_border, INTERNAL_BORDER_FACE_ID);
+      realize_named_face (f, Qchild_frame_border, CHILD_FRAME_BORDER_FACE_ID);
       realize_named_face (f, Qtab_bar, TAB_BAR_FACE_ID);
       realize_named_face (f, Qtab_line, TAB_LINE_FACE_ID);

@@ -6967,6 +6969,7 @@ syms_of_xfaces (void)
   DEFSYM (Qwindow_divider_first_pixel, "window-divider-first-pixel");
   DEFSYM (Qwindow_divider_last_pixel, "window-divider-last-pixel");
   DEFSYM (Qinternal_border, "internal-border");
+  DEFSYM (Qchild_frame_border, "child-frame-border");

   /* TTY color-related functions (defined in tty-colors.el).  */
   DEFSYM (Qtty_color_desc, "tty-color-desc");
diff --git a/src/xterm.c b/src/xterm.c
index 0a86738cc2..92dcd95043 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -1293,9 +1293,13 @@ x_clear_under_internal_border (struct frame *f)
       int height = FRAME_PIXEL_HEIGHT (f);
       int margin = FRAME_TOP_MARGIN_HEIGHT (f);
       int face_id =
-	!NILP (Vface_remapping_alist)
-	? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
-	: INTERNAL_BORDER_FACE_ID;
+	(FRAME_PARENT_FRAME (f)
+	 ? (!NILP (Vface_remapping_alist)
+	    ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
+	    : CHILD_FRAME_BORDER_FACE_ID)
+	 : (!NILP (Vface_remapping_alist)
+	    ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
+	    : INTERNAL_BORDER_FACE_ID));
       struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);

       block_input ();
@@ -1360,9 +1364,13 @@ x_after_update_window_line (struct window *w, struct glyph_row *desired_row)
       {
 	int y = WINDOW_TO_FRAME_PIXEL_Y (w, max (0, desired_row->y));
 	int face_id =
-	  !NILP (Vface_remapping_alist)
-	  ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
-	  : INTERNAL_BORDER_FACE_ID;
+	  (FRAME_PARENT_FRAME (f)
+	   ? (!NILP (Vface_remapping_alist)
+	      ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
+	      : CHILD_FRAME_BORDER_FACE_ID)
+	   : (!NILP (Vface_remapping_alist)
+	      ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
+	      : INTERNAL_BORDER_FACE_ID));
 	struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);

 	block_input ();
--
2.29.2


[-- Attachment #3: 0002-WIP-child-frame-border-width.patch --]
[-- Type: text/x-patch, Size: 5925 bytes --]

From b176909995eea600d99354be2127469f472ce4bb Mon Sep 17 00:00:00 2001
From: Alexander Miller <alexanderm@web.de>
Date: Tue, 5 Jan 2021 13:31:09 +0100
Subject: [PATCH 2/2] WIP child-frame-border-width

---
 src/frame.c |  4 ++++
 src/frame.h | 14 +++++++++++++-
 src/xfns.c  | 54 +++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/src/frame.c b/src/frame.c
index 45ee96e962..d5040dfae6 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -3744,6 +3744,7 @@ DEFUN ("set-frame-window-state-change", Fset_frame_window_state_change,
   {"foreground-color",		-1},
   {"icon-name",			SYMBOL_INDEX (Qicon_name)},
   {"icon-type",			SYMBOL_INDEX (Qicon_type)},
+  {"child-frame-border-width",	SYMBOL_INDEX (Qchild_frame_border_width)},
   {"internal-border-width",	SYMBOL_INDEX (Qinternal_border_width)},
   {"right-divider-width",	SYMBOL_INDEX (Qright_divider_width)},
   {"bottom-divider-width",	SYMBOL_INDEX (Qbottom_divider_width)},
@@ -4287,6 +4288,8 @@ gui_report_frame_params (struct frame *f, Lisp_Object *alistptr)

   store_in_alist (alistptr, Qborder_width,
 		  make_fixnum (f->border_width));
+  store_in_alist (alistptr, Qchild_frame_border_width,
+		  make_fixnum (CHILD_FRAME_BORDER_WIDTH (f)));
   store_in_alist (alistptr, Qinternal_border_width,
 		  make_fixnum (FRAME_INTERNAL_BORDER_WIDTH (f)));
   store_in_alist (alistptr, Qright_divider_width,
@@ -5984,6 +5987,7 @@ syms_of_frame (void)
   DEFSYM (Qhorizontal_scroll_bars, "horizontal-scroll-bars");
   DEFSYM (Qicon_name, "icon-name");
   DEFSYM (Qicon_type, "icon-type");
+  DEFSYM (Qchild_frame_border_width, "child-frame-border-width");
   DEFSYM (Qinternal_border_width, "internal-border-width");
   DEFSYM (Qleft_fringe, "left-fringe");
   DEFSYM (Qline_spacing, "line-spacing");
diff --git a/src/frame.h b/src/frame.h
index 8cf41dc004..7d816f2516 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -534,6 +534,10 @@ #define EMACS_FRAME_H
   /* Border width of the frame window as known by the (X) window system.  */
   int border_width;

+  /* Width of child frames' internal border.  Acts exactly like
+     internal_border_width below and will fall back on it when not set.*/
+  int child_frame_border_width;
+
   /* Width of the internal border.  This is a line of background color
      just inside the window's border.  When the frame is selected,
      a highlighting is displayed inside the internal border.  */
@@ -1432,11 +1436,19 @@ FRAME_TOTAL_FRINGE_WIDTH (struct frame *f)
   return FRAME_LEFT_FRINGE_WIDTH (f) + FRAME_RIGHT_FRINGE_WIDTH (f);
 }

+INLINE int
+CHILD_FRAME_BORDER_WIDTH (struct frame *f)
+{
+  return frame_dimension (f->child_frame_border_width);
+}
+
 /* Pixel-width of internal border lines.  */
 INLINE int
 FRAME_INTERNAL_BORDER_WIDTH (struct frame *f)
 {
-  return frame_dimension (f->internal_border_width);
+  return f->child_frame_border_width
+    ? CHILD_FRAME_BORDER_WIDTH(f)
+    : frame_dimension (f->internal_border_width);
 }

 /* Pixel-size of window divider lines.  */
diff --git a/src/xfns.c b/src/xfns.c
index 9ab537ca8d..663146d7f2 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -1800,6 +1800,28 @@ x_change_tool_bar_height (struct frame *f, int height)
 #endif /* USE_GTK */
 }

+static void
+x_set_child_frame_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
+{
+  int border = check_int_nonnegative (arg);
+
+  if (border != CHILD_FRAME_BORDER_WIDTH (f))
+    {
+      f->child_frame_border_width = border;
+
+#ifdef USE_X_TOOLKIT
+      if (FRAME_X_OUTPUT (f)->edit_widget)
+	widget_store_internal_border (FRAME_X_OUTPUT (f)->edit_widget);
+#endif
+
+      if (FRAME_X_WINDOW (f))
+	{
+	  adjust_frame_size (f, -1, -1, 3, false, Qchild_frame_border_width);
+	  x_clear_under_internal_border (f);
+	}
+    }
+
+}

 static void
 x_set_internal_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
@@ -3897,14 +3919,37 @@ DEFUN ("x-create-frame", Fx_create_frame, Sx_create_frame,
 	parms = Fcons (Fcons (Qinternal_border_width, value),
 		       parms);
     }
+  /* Same for child frames.  */
+  if (NILP (Fassq (Qchild_frame_border_width, parms)))
+    {
+      Lisp_Object value;
+
+      value = gui_display_get_arg (dpyinfo, parms, Qchild_frame_border_width,
+                                   "internalBorder", "internalBorder",
+                                   RES_TYPE_NUMBER);
+      if (! EQ (value, Qunbound))
+	parms = Fcons (Fcons (Qchild_frame_border_width, value),
+		       parms);
+
+    }
+
+  gui_default_parameter (f, parms, Qchild_frame_border_width,
+#ifdef USE_GTK /* We used to impose 0 in xg_create_frame_widgets.  */
+			 make_fixnum (0),
+#else
+			 make_fixnum (1),
+#endif
+			 "childFrameBorderWidth", "childFrameBorderWidth",
+			 RES_TYPE_NUMBER);
   gui_default_parameter (f, parms, Qinternal_border_width,
 #ifdef USE_GTK /* We used to impose 0 in xg_create_frame_widgets.  */
-                         make_fixnum (0),
+			 make_fixnum (0),
 #else
-                         make_fixnum (1),
+			 make_fixnum (1),
 #endif
-                         "internalBorderWidth", "internalBorderWidth",
-                         RES_TYPE_NUMBER);
+			 "internalBorderWidth", "internalBorderWidth",
+			 RES_TYPE_NUMBER);
+
   gui_default_parameter (f, parms, Qright_divider_width, make_fixnum (0),
                          NULL, NULL, RES_TYPE_NUMBER);
   gui_default_parameter (f, parms, Qbottom_divider_width, make_fixnum (0),
@@ -7762,6 +7807,7 @@ DEFUN ("x-gtk-debug", Fx_gtk_debug, Sx_gtk_debug, 1, 1, 0,
   x_set_foreground_color,
   x_set_icon_name,
   x_set_icon_type,
+  x_set_child_frame_border_width,
   x_set_internal_border_width,
   gui_set_right_divider_width,
   gui_set_bottom_divider_width,
--
2.29.2

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 > To be honest I've don't really like C as a language, and learning it
 > would not do anything professionally for me either. So I'm jumping into
 > this strictly for the sake of improving Emacs.

Then please jump into this as often as you want.

 > x, ns and w32 all used the same code, so I changed them all to look like
 > this now.

OK.  I think a main reason that we usually do not factor out code in
these is that only one of them gets compiled in anyway - so we just save
an extra call.

 >  >> - Currently the actual width of the border is still controlled by the
 >  >> `internal-border-width` parameter for both frame types. Should I try to
 >  >> do something about that as well? If yes, what's my entry point?
 >  >
 >  > Add a 'child-frame-border-width' parameter. But in this case I would
 >  > propose to proceed as follows:
 >  >
 >  > - If for a frame the 'child-frame-border-width' was explicitly set, use
 >  > it.
 >  >
 >  > - If it was not set, use the 'internal-border-width' parameter.
 >
 > That's done now too, at least for X. I'll attach patches of my first
 > working drafts for both changes. Let me know if I'm on the right path so
 > far.

You're already at the end of it.  Please document the changes in the
Elisp manual and announce them as (probably incompatible) changes in
NEWS.

 >  >> - I think I'll need to sign the FSF copyright assignment, unless the
 >  >> limit is higher than the 15 lines I am remembering.
 >  >
 >  > I think so too.
 >  >
 >
 > Ok, what do I do?

Eli, can you send Arthur the forms please?

Thanks, martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

> Eli, can you send Arthur the forms please?
                     ^^^^^

Alexander, obviously ...

martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Eli Zaretskii]

> Cc: 45620@debbugs.gnu.org, tumashu@163.com, Eli Zaretskii <eliz@gnu.org>
> From: martin rudalics <rudalics@gmx.at>
> Date: Tue, 5 Jan 2021 16:33:05 +0100
> 
> Eli, can you send Arthur the forms please?

Done.

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Arthur Miller]

martin rudalics <rudalics@gmx.at> writes:

>> Eli, can you send Arthur the forms please?
>                     ^^^^^
>
> Alexander, obviously ...
>
> martin
Arthur already signed and prefer borderless stuff ... :-)

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

> Arthur already signed and prefer borderless stuff ... :-)

Waiting for Henry to show up ...

martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Arthur Miller]

martin rudalics <rudalics@gmx.at> writes:

>> Arthur already signed and prefer borderless stuff ... :-)
>
> Waiting for Henry to show up ...
>
> martin

And if Glenn appears we could have a jazz quartet!

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alexander Miller]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

 >> >> - Currently the actual width of the border is still controlled by
the
 >> >> `internal-border-width` parameter for both frame types. Should I
try to
 >> >> do something about that as well? If yes, what's my entry point?
 >> >
 >> > Add a 'child-frame-border-width' parameter. But in this case I would
 >> > propose to proceed as follows:
 >> >
 >> > - If for a frame the 'child-frame-border-width' was explicitly
set, use
 >> > it.
 >> >
 >> > - If it was not set, use the 'internal-border-width' parameter.
 >>
 >> That's done now too, at least for X. I'll attach patches of my first
 >> working drafts for both changes. Let me know if I'm on the right
path so
 >> far.
 >
 > You're already at the end of it. Please document the changes in the
 > Elisp manual and announce them as (probably incompatible) changes in
 > NEWS.
 >

Ok, all done now, see attached patch.

For the copyright I have asked my employer to make sure that the
copyright parts from my contract don't apply to the work I do on my own
time like this, but most of the company will still be on holidays or on
sick
leave, so it could be a few days before I get an answer.


[-- Attachment #2: 0001-Add-distinct-controls-for-the-appearance-of-child-fr.patch --]
[-- Type: text/x-patch, Size: 19453 bytes --]

From 623b6ab24b93ca21e7fa32dd344955d1138c157f Mon Sep 17 00:00:00 2001
From: Alexander Miller <alexanderm@web.de>
Date: Wed, 6 Jan 2021 17:02:35 +0100
Subject: [PATCH] Add distinct controls for the appearance of child frames'
 borders.

The background of the `child-frame-border` face instead of the
`internal-border` face now controls the color of child frames'
borders.

The `child-frame-border-width` frame parameter is now used for the
width of child frames' borders instead of `internal-border-width`,
though we still fall back on using the latter if the former is not
set.
---
 doc/lispref/frames.texi | 19 +++++++++++---
 etc/NEWS                |  7 +++++
 lisp/faces.el           | 11 +++++++-
 src/dispextern.h        |  1 +
 src/frame.c             | 12 +++++++++
 src/frame.h             | 25 ++++++++++++++++--
 src/nsfns.m             | 18 +++++++++++++
 src/nsterm.m            | 10 ++++---
 src/w32fns.c            | 58 ++++++++++++++++++++++++++++++++++++++---
 src/xfaces.c            |  3 +++
 src/xfns.c              | 46 ++++++++++++++++++++++++++++++++
 src/xterm.c             | 20 +++++++++-----
 12 files changed, 212 insertions(+), 18 deletions(-)

diff --git a/doc/lispref/frames.texi b/doc/lispref/frames.texi
index 7f2a6f7542..ef1b661b2a 100644
--- a/doc/lispref/frames.texi
+++ b/doc/lispref/frames.texi
@@ -694,9 +694,17 @@ Frame Layout

 @item Internal Border
 The internal border is a border drawn by Emacs around the inner frame
-(see below).  Its width is specified by the @code{internal-border-width}
-frame parameter (@pxref{Layout Parameters}).  Its color is specified by
-the background of the @code{internal-border} face.
+(see below). The specification of its appearance depends on whether
+the given frame is a child frame (@pxref{Child Frames}) or not.
+
+For normal frames its width is specified by the @code{internal-border-width}
+frame parameter (@pxref{Layout Parameters}) and its color is specified by the
+background of the @code{internal-border} face.
+
+For child frames its width is specified by the @code{child-frame-border-width}
+frame parameter (but will use the the @code{internal-border-width} parameter as
+a fallback) and its color is specified by the background of the
+@code{child-frame-border} face.

 @item Inner Frame
 @cindex inner frame
@@ -1790,6 +1798,11 @@ Layout Parameters
 The width in pixels of the frame's internal border (@pxref{Frame
 Geometry}).

+@vindex child-frame-border-width@r{, a frame parameter}
+@item child-frame-border-width
+The width in pixels of the frame's internal border (@pxref{Frame
+Geometry}) if the given frame is a child frame (@pxref{Child Frames}).
+
 @vindex vertical-scroll-bars@r{, a frame parameter}
 @item vertical-scroll-bars
 Whether the frame has scroll bars (@pxref{Scroll Bars}) for vertical
diff --git a/etc/NEWS b/etc/NEWS
index d8f25ab362..f1e8443080 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -218,6 +218,13 @@ and other "slow scrolling" situations.  It is hoped it behaves better
 than 'fast-but-imprecise-scrolling' and 'jit-lock-defer-time'.
 It is not enabled by default.

+** New face 'child-frame-border' and new frame parameter child-frame-border-width
+The face and width of child frames can no be determined separately from normal
+frames. To minimize backwards incompatibility child-frames without a
+'child-frame-border-width' parameter will fallback to using 'internal-border-width'.
+However the new 'child-frame-border' face does constitute a breaking change since
+child frames' borders no longer use the 'internal-border' face.
+
 \f
 * Editing Changes in Emacs 28.1

diff --git a/lisp/faces.el b/lisp/faces.el
index 4e98338432..639dbeb09a 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2683,11 +2683,20 @@ window-divider-last-pixel

 (defface internal-border
     '((t nil))
-  "Basic face for the internal border."
+  "Basic face for the internal border.
+For the internal border of child frames see `child-frame-border'."
   :version "26.1"
   :group 'frames
   :group 'basic-faces)

+(defface child-frame-border
+  '((t nil))
+  "Basic face for the internal border of child frames.
+For the internal border of non-child frames see `internal-border'."
+  :version "28.1"
+  :group 'frames
+  :group 'basic-faces)
+
 (defface minibuffer-prompt
   '((((background dark)) :foreground "cyan")
     ;; Don't use blue because many users of the MS-DOS port customize
diff --git a/src/dispextern.h b/src/dispextern.h
index 3ad98b8344..f4e872644d 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1826,6 +1826,7 @@ #define FACE_UNIBYTE_P(FACE) ((FACE)->charset < 0)
   WINDOW_DIVIDER_FIRST_PIXEL_FACE_ID,
   WINDOW_DIVIDER_LAST_PIXEL_FACE_ID,
   INTERNAL_BORDER_FACE_ID,
+  CHILD_FRAME_BORDER_FACE_ID,
   TAB_BAR_FACE_ID,
   TAB_LINE_FACE_ID,
   BASIC_FACE_ID_SENTINEL
diff --git a/src/frame.c b/src/frame.c
index 45ee96e962..4a4ad99395 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -3528,6 +3528,13 @@ DEFUN ("frame-fringe-width", Ffringe_width, Sfringe_width, 0, 1, 0,
   return make_fixnum (FRAME_TOTAL_FRINGE_WIDTH (decode_any_frame (frame)));
 }

+DEFUN ("frame-child-frame-border-width", Fframe_child_frame_border_width, Sframe_child_frame_border_width, 0, 1, 0,
+       doc: /* Return width of FRAME's child-frame border in pixels.  */)
+  (Lisp_Object frame)
+{
+  return make_fixnum (FRAME_CHILD_FRAME_BORDER_WIDTH (decode_any_frame (frame)));
+}
+
 DEFUN ("frame-internal-border-width", Fframe_internal_border_width, Sframe_internal_border_width, 0, 1, 0,
        doc: /* Return width of FRAME's internal border in pixels.  */)
   (Lisp_Object frame)
@@ -3744,6 +3751,7 @@ DEFUN ("set-frame-window-state-change", Fset_frame_window_state_change,
   {"foreground-color",		-1},
   {"icon-name",			SYMBOL_INDEX (Qicon_name)},
   {"icon-type",			SYMBOL_INDEX (Qicon_type)},
+  {"child-frame-border-width",	SYMBOL_INDEX (Qchild_frame_border_width)},
   {"internal-border-width",	SYMBOL_INDEX (Qinternal_border_width)},
   {"right-divider-width",	SYMBOL_INDEX (Qright_divider_width)},
   {"bottom-divider-width",	SYMBOL_INDEX (Qbottom_divider_width)},
@@ -4287,6 +4295,8 @@ gui_report_frame_params (struct frame *f, Lisp_Object *alistptr)

   store_in_alist (alistptr, Qborder_width,
 		  make_fixnum (f->border_width));
+  store_in_alist (alistptr, Qchild_frame_border_width,
+		  make_fixnum (FRAME_CHILD_FRAME_BORDER_WIDTH (f)));
   store_in_alist (alistptr, Qinternal_border_width,
 		  make_fixnum (FRAME_INTERNAL_BORDER_WIDTH (f)));
   store_in_alist (alistptr, Qright_divider_width,
@@ -5984,6 +5994,7 @@ syms_of_frame (void)
   DEFSYM (Qhorizontal_scroll_bars, "horizontal-scroll-bars");
   DEFSYM (Qicon_name, "icon-name");
   DEFSYM (Qicon_type, "icon-type");
+  DEFSYM (Qchild_frame_border_width, "child-frame-border-width");
   DEFSYM (Qinternal_border_width, "internal-border-width");
   DEFSYM (Qleft_fringe, "left-fringe");
   DEFSYM (Qline_spacing, "line-spacing");
@@ -6408,6 +6419,7 @@ focus (where a frame immediately loses focus when it's left by the mouse
   defsubr (&Sscroll_bar_width);
   defsubr (&Sscroll_bar_height);
   defsubr (&Sfringe_width);
+  defsubr (&Sframe_child_frame_border_width);
   defsubr (&Sframe_internal_border_width);
   defsubr (&Sright_divider_width);
   defsubr (&Sbottom_divider_width);
diff --git a/src/frame.h b/src/frame.h
index 8cf41dc004..6a94cee727 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -534,6 +534,10 @@ #define EMACS_FRAME_H
   /* Border width of the frame window as known by the (X) window system.  */
   int border_width;

+  /* Width of child frames' internal border. Acts as the
+     interal_border_width foy child frames.  */
+  int child_frame_border_width;
+
   /* Width of the internal border.  This is a line of background color
      just inside the window's border.  When the frame is selected,
      a highlighting is displayed inside the internal border.  */
@@ -1432,11 +1436,28 @@ FRAME_TOTAL_FRINGE_WIDTH (struct frame *f)
   return FRAME_LEFT_FRINGE_WIDTH (f) + FRAME_RIGHT_FRINGE_WIDTH (f);
 }

-/* Pixel-width of internal border lines.  */
+INLINE int
+FRAME_CHILD_FRAME_BORDER_WIDTH (struct frame *f)
+{
+  return frame_dimension (f->child_frame_border_width);
+}
+
+/* Pixel-width of internal border lines.
+   Will use child_frame_border_width for child frames if
+   possible, and fall back on internal_border_width
+   otherwise.  */
 INLINE int
 FRAME_INTERNAL_BORDER_WIDTH (struct frame *f)
 {
-  return frame_dimension (f->internal_border_width);
+#ifdef HAVE_WINDOW_SYSTEM
+  return FRAME_PARENT_FRAME(f)
+    ? (f->child_frame_border_width
+       ? FRAME_CHILD_FRAME_BORDER_WIDTH(f)
+       : frame_dimension (f->internal_border_width))
+    : frame_dimension (f->internal_border_width);
+#else
+  return frame_dimension (f->internal_border_width)
+#endif
 }

 /* Pixel-size of window divider lines.  */
diff --git a/src/nsfns.m b/src/nsfns.m
index ae114f83e4..1a81b07aae 100644
--- a/src/nsfns.m
+++ b/src/nsfns.m
@@ -687,6 +687,21 @@ Turn the input menu (an NSMenu) into a lisp list for tracking on lisp side.
   }
 }

+static void
+ns_set_child_frame_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
+{
+  int old_width = FRAME_CHILD_FRAME_BORDER_WIDTH (f);
+  int new_width = check_int_nonnegative (arg);
+
+  if (new_width == old_width)
+    return;
+  f->child_frame_border_width = new_width;
+
+  if (FRAME_NATIVE_WINDOW (f) != 0)
+    adjust_frame_size (f, -1, -1, 3, 0, Qchild_frame_border_width);
+
+  SET_FRAME_GARBAGED (f);
+}

 static void
 ns_set_internal_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
@@ -1197,6 +1212,9 @@ Turn the input menu (an NSMenu) into a lisp list for tracking on lisp side.
   gui_default_parameter (f, parms, Qinternal_border_width, make_fixnum (2),
                          "internalBorderWidth", "InternalBorderWidth",
                          RES_TYPE_NUMBER);
+  gui_default_parameter (f, parms, Qchild_frame_border_width, make_fixnum (2),
+			 "childFrameBorderWidth", "childFrameBorderWidth",
+			 RES_TYPE_NUMBER);
   gui_default_parameter (f, parms, Qright_divider_width, make_fixnum (0),
 		       NULL, NULL, RES_TYPE_NUMBER);
   gui_default_parameter (f, parms, Qbottom_divider_width, make_fixnum (0),
diff --git a/src/nsterm.m b/src/nsterm.m
index 2defb9e2ee..76f67d2531 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3029,9 +3029,13 @@ so some key presses (TAB) are swallowed by the system.  */
       NSRectEdge edge[] = {NSMinXEdge, NSMinYEdge, NSMaxXEdge, NSMaxYEdge};

       int face_id =
-	!NILP (Vface_remapping_alist)
-	? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
-	: INTERNAL_BORDER_FACE_ID;
+        (FRAME_PARENT_FRAME (f)
+         ? (!NILP (Vface_remapping_alist)
+            ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
+            : CHILD_FRAME_BORDER_FACE_ID)
+         : (!NILP (Vface_remapping_alist)
+            ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
+            : INTERNAL_BORDER_FACE_ID));
       struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);

       if (!face)
diff --git a/src/w32fns.c b/src/w32fns.c
index c1e18ff7fa..1d3fb5d761 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -1519,9 +1519,13 @@ w32_clear_under_internal_border (struct frame *f)
       int width = FRAME_PIXEL_WIDTH (f);
       int height = FRAME_PIXEL_HEIGHT (f);
       int face_id =
-	!NILP (Vface_remapping_alist)
-	? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
-	: INTERNAL_BORDER_FACE_ID;
+	(FRAME_PARENT_FRAME (f)
+	 ? (!NILP (Vface_remapping_alist)
+	    ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
+	    : CHILD_FRAME_BORDER_FACE_ID)
+	 : (!NILP (Vface_remapping_alist)
+	    ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
+	    : INTERNAL_BORDER_FACE_ID));
       struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);

       block_input ();
@@ -1548,6 +1552,32 @@ w32_clear_under_internal_border (struct frame *f)
     }
 }

+/**
+ * w32_set_child_frame_border_width:
+ *
+ * Set width of child frame F's internal border to ARG pixels.
+ * ARG < 0 is * treated like ARG = 0.
+ */
+static void
+w32_set_internal_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
+{
+  int argval = check_integer_range (arg, INT_MIN, INT_MAX);
+  int border = max (argval, 0);
+
+  if (border != FRAME_CHILD_FRAME_BORDER_WIDTH (f))
+    {
+      f->child_frame_border_width = border;
+
+      if (FRAME_NATIVE_WINDOW (f) != 0)
+	{
+	  adjust_frame_size (f, -1, -1, 3, false, Qchild_frame_border_width);
+
+	  if (FRAME_VISIBLE_P (f))
+	    w32_clear_under_internal_border (f);
+	}
+    }
+}
+

 /**
  * w32_set_internal_border_width:
@@ -5873,6 +5903,28 @@ DEFUN ("x-create-frame", Fx_create_frame, Sx_create_frame,
 			    parameters);
     }

+  /* Same for child frames.  */
+  if (NILP (Fassq (Qchild_frame_border_width, parameters)))
+    {
+      Lisp_Object value;
+
+      value = gui_display_get_arg (dpyinfo, parameters, Qchild_frame_border_width,
+                                   "childFrameBorderWidth", "childFrameBorderWidth",
+                                   RES_TYPE_NUMBER);
+      if (! EQ (value, Qunbound))
+	parameters = Fcons (Fcons (Qchild_frame_border_width, value),
+		       parameters);
+
+    }
+
+  gui_default_parameter (f, parameters, Qchild_frame_border_width,
+#ifdef USE_GTK /* We used to impose 0 in xg_create_frame_widgets.  */
+			 make_fixnum (0),
+#else
+			 make_fixnum (1),
+#endif
+			 "childFrameBorderWidth", "childFrameBorderWidth",
+			 RES_TYPE_NUMBER);
   gui_default_parameter (f, parameters, Qinternal_border_width, make_fixnum (0),
                          "internalBorderWidth", "InternalBorder", RES_TYPE_NUMBER);
   gui_default_parameter (f, parameters, Qright_divider_width, make_fixnum (0),
diff --git a/src/xfaces.c b/src/xfaces.c
index b3b19a9cb2..5080120957 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -4913,6 +4913,7 @@ lookup_basic_face (struct window *w, struct frame *f, int face_id)
     case WINDOW_DIVIDER_FIRST_PIXEL_FACE_ID:	name = Qwindow_divider_first_pixel;	break;
     case WINDOW_DIVIDER_LAST_PIXEL_FACE_ID:	name = Qwindow_divider_last_pixel;	break;
     case INTERNAL_BORDER_FACE_ID:	name = Qinternal_border; 	break;
+    case CHILD_FRAME_BORDER_FACE_ID:	name = Qchild_frame_border; 	break;

     default:
       emacs_abort (); /* the caller is supposed to pass us a basic face id */
@@ -5619,6 +5620,7 @@ realize_basic_faces (struct frame *f)
       realize_named_face (f, Qwindow_divider_last_pixel,
 			  WINDOW_DIVIDER_LAST_PIXEL_FACE_ID);
       realize_named_face (f, Qinternal_border, INTERNAL_BORDER_FACE_ID);
+      realize_named_face (f, Qchild_frame_border, CHILD_FRAME_BORDER_FACE_ID);
       realize_named_face (f, Qtab_bar, TAB_BAR_FACE_ID);
       realize_named_face (f, Qtab_line, TAB_LINE_FACE_ID);

@@ -6967,6 +6969,7 @@ syms_of_xfaces (void)
   DEFSYM (Qwindow_divider_first_pixel, "window-divider-first-pixel");
   DEFSYM (Qwindow_divider_last_pixel, "window-divider-last-pixel");
   DEFSYM (Qinternal_border, "internal-border");
+  DEFSYM (Qchild_frame_border, "child-frame-border");

   /* TTY color-related functions (defined in tty-colors.el).  */
   DEFSYM (Qtty_color_desc, "tty-color-desc");
diff --git a/src/xfns.c b/src/xfns.c
index 9ab537ca8d..cac41ee485 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -1800,6 +1800,28 @@ x_change_tool_bar_height (struct frame *f, int height)
 #endif /* USE_GTK */
 }

+static void
+x_set_child_frame_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
+{
+  int border = check_int_nonnegative (arg);
+
+  if (border != FRAME_CHILD_FRAME_BORDER_WIDTH (f))
+    {
+      f->child_frame_border_width = border;
+
+#ifdef USE_X_TOOLKIT
+      if (FRAME_X_OUTPUT (f)->edit_widget)
+	widget_store_internal_border (FRAME_X_OUTPUT (f)->edit_widget);
+#endif
+
+      if (FRAME_X_WINDOW (f))
+	{
+	  adjust_frame_size (f, -1, -1, 3, false, Qchild_frame_border_width);
+	  x_clear_under_internal_border (f);
+	}
+    }
+
+}

 static void
 x_set_internal_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
@@ -3897,6 +3919,29 @@ DEFUN ("x-create-frame", Fx_create_frame, Sx_create_frame,
 	parms = Fcons (Fcons (Qinternal_border_width, value),
 		       parms);
     }
+
+  /* Same for child frames.  */
+  if (NILP (Fassq (Qchild_frame_border_width, parms)))
+    {
+      Lisp_Object value;
+
+      value = gui_display_get_arg (dpyinfo, parms, Qchild_frame_border_width,
+                                   "childFrameBorderWidth", "childFrameBorderWidth",
+                                   RES_TYPE_NUMBER);
+      if (! EQ (value, Qunbound))
+	parms = Fcons (Fcons (Qchild_frame_border_width, value),
+		       parms);
+
+    }
+
+  gui_default_parameter (f, parms, Qchild_frame_border_width,
+#ifdef USE_GTK /* We used to impose 0 in xg_create_frame_widgets.  */
+			 make_fixnum (0),
+#else
+			 make_fixnum (1),
+#endif
+			 "childFrameBorderWidth", "childFrameBorderWidth",
+			 RES_TYPE_NUMBER);
   gui_default_parameter (f, parms, Qinternal_border_width,
 #ifdef USE_GTK /* We used to impose 0 in xg_create_frame_widgets.  */
                          make_fixnum (0),
@@ -7762,6 +7807,7 @@ DEFUN ("x-gtk-debug", Fx_gtk_debug, Sx_gtk_debug, 1, 1, 0,
   x_set_foreground_color,
   x_set_icon_name,
   x_set_icon_type,
+  x_set_child_frame_border_width,
   x_set_internal_border_width,
   gui_set_right_divider_width,
   gui_set_bottom_divider_width,
diff --git a/src/xterm.c b/src/xterm.c
index 0a86738cc2..92dcd95043 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -1293,9 +1293,13 @@ x_clear_under_internal_border (struct frame *f)
       int height = FRAME_PIXEL_HEIGHT (f);
       int margin = FRAME_TOP_MARGIN_HEIGHT (f);
       int face_id =
-	!NILP (Vface_remapping_alist)
-	? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
-	: INTERNAL_BORDER_FACE_ID;
+	(FRAME_PARENT_FRAME (f)
+	 ? (!NILP (Vface_remapping_alist)
+	    ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
+	    : CHILD_FRAME_BORDER_FACE_ID)
+	 : (!NILP (Vface_remapping_alist)
+	    ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
+	    : INTERNAL_BORDER_FACE_ID));
       struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);

       block_input ();
@@ -1360,9 +1364,13 @@ x_after_update_window_line (struct window *w, struct glyph_row *desired_row)
       {
 	int y = WINDOW_TO_FRAME_PIXEL_Y (w, max (0, desired_row->y));
 	int face_id =
-	  !NILP (Vface_remapping_alist)
-	  ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
-	  : INTERNAL_BORDER_FACE_ID;
+	  (FRAME_PARENT_FRAME (f)
+	   ? (!NILP (Vface_remapping_alist)
+	      ? lookup_basic_face (NULL, f, CHILD_FRAME_BORDER_FACE_ID)
+	      : CHILD_FRAME_BORDER_FACE_ID)
+	   : (!NILP (Vface_remapping_alist)
+	      ? lookup_basic_face (NULL, f, INTERNAL_BORDER_FACE_ID)
+	      : INTERNAL_BORDER_FACE_ID));
 	struct face *face = FACE_FROM_ID_OR_NULL (f, face_id);

 	block_input ();
--
2.29.2

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 > Ok, all done now, see attached patch.

Looks good.

 > For the copyright I have asked my employer to make sure that the
 > copyright parts from my contract don't apply to the work I do on my own
 > time like this, but most of the company will still be on holidays or on
 > sick
 > leave, so it could be a few days before I get an answer.

Let's hope for the best.

Thanks, martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alexander Miller]

 >> For the copyright I have asked my employer to make sure that the
 >> copyright parts from my contract don't apply to the work I do on my own
 >> time like this, but most of the company will still be on holidays or on
 >> sick
 >> leave, so it could be a few days before I get an answer.
 >
 > Let's hope for the best.
 >
 > Thanks, martin

I got green light from my employer and sent the signed papers today -
we're good to go for whatever comes next.

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 > I got green light from my employer and sent the signed papers today -
 > we're good to go for whatever comes next.

Fine.  When the FSF gets back to you please tell us.

Thanks, martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alexander Miller]

 >> I got green light from my employer and sent the signed papers today -
 >> we're good to go for whatever comes next.
 >
 > Fine. When the FSF gets back to you please tell us.
 >
 > Thanks, martin

They got back to me, I have received my signed copy of the documents now.

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 > They got back to me, I have received my signed copy of the documents now.

I hope to install your patch tomorrow.  If you want to ensure a precise
position for the NEWS entry, you'd have to update it though.  Otherwise,
I'll try to find a suitable position myself.

Thanks, martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 > I hope to install your patch tomorrow.  If you want to ensure a precise
 > position for the NEWS entry, you'd have to update it though.  Otherwise,
 > I'll try to find a suitable position myself.

I pushed the changes now with a few tweaks.  Please have a look and
consider closing the bug if you feel that nothing else should be done.

I couldn't test the changes with GNUstep here, those builds still fail
to behave reasonably on my machine.  So if you know people who regularly
build Emacs with NS, please ask them to check the changes.

Thanks, martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alexander Miller]

 >> I hope to install your patch tomorrow. If you want to ensure a precise
 >> position for the NEWS entry, you'd have to update it though. Otherwise,
 >> I'll try to find a suitable position myself.
 >
 > I pushed the changes now with a few tweaks. Please have a look and
 > consider closing the bug if you feel that nothing else should be done.

I have tested the changes locally now, and everything is working as
expected. I'd close the bug, but I don't know how. I have found some 12
year old documentation that says to send my mail to
nnn-done@debbugs.gnu.org. Is that still correct, or will my mail just
get lost?

 > I couldn't test the changes with GNUstep here, those builds still fail
 > to behave reasonably on my machine. So if you know people who regularly
 > build Emacs with NS, please ask them to check the changes.

I have gotten one or two people into Emacs to some degree over the
years, but I am the most hardcore user I know by far, so I'm afraid I
can't help with that.

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alan Third]

On Tue, Jan 26, 2021 at 04:59:13PM +0100, martin rudalics wrote:
> > I hope to install your patch tomorrow.  If you want to ensure a precise
> > position for the NEWS entry, you'd have to update it though.  Otherwise,
> > I'll try to find a suitable position myself.
> 
> I pushed the changes now with a few tweaks.  Please have a look and
> consider closing the bug if you feel that nothing else should be done.
> 
> I couldn't test the changes with GNUstep here, those builds still fail
> to behave reasonably on my machine.  So if you know people who regularly
> build Emacs with NS, please ask them to check the changes.

Is there some simple test I can try? I had a quick look through the
thread but didn't see anything.

(GNUstep builds work for me as long as I start with a .emacs that
turns off the menus. I can't work out what it is that's causing the
problem, and even the large rewrite of some of the menu code we did on
the master branch makes no difference. It's quite annoying.)

-- 
Alan Third

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Eli Zaretskii]

> From: Alexander Miller <alexanderm@web.de>
> Date: Wed, 27 Jan 2021 21:44:21 +0100
> Cc: 45620@debbugs.gnu.org, tumashu@163.com
> 
> I'd close the bug, but I don't know how. I have found some 12
> year old documentation that says to send my mail to
> nnn-done@debbugs.gnu.org. Is that still correct, or will my mail just
> get lost?

This is still correct.

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alexander Miller]

 >> From: Alexander Miller <alexanderm@web.de>
 >> Date: Wed, 27 Jan 2021 21:44:21 +0100
 >> Cc: 45620@debbugs.gnu.org, tumashu@163.com
 >>
 >> I'd close the bug, but I don't know how. I have found some 12
 >> year old documentation that says to send my mail to
 >> nnn-done@debbugs.gnu.org. Is that still correct, or will my mail just
 >> get lost?
 >
 > This is still correct.

All right, let's close this bug then.

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 > Is there some simple test I can try? I had a quick look through the
 > thread but didn't see anything.

Please with emacs -Q define

(defun my-make-child-frame ()
   (interactive)
   (make-frame
    `((parent-frame . ,(selected-frame))
      (undecorated . t)
      (left . 0.5)
      (top . 0.5)
      (width . 0.3)
      (height . 0.3)
      (internal-border-width . 3))))

then do

(setq frame (my-make-child-frame))

and finally perform the following two experiments:

(1) Customize the background of the faces 'internal-border' and
'child-frame-border'.  Only customizing the latter should affect the
child frame.

(2) Do

(set-frame-parameter frame 'internal-border-width 7)
(set-frame-parameter frame 'child-frame-border-width 1)

Either of them should change the border width of the child frame as
indicated.  If you now do

(set-frame-parameter nil 'internal-border-width 5)

with your normal frame selected, that frame's internal border width
should change but the child frame's border width should remain
unaltered.

 > (GNUstep builds work for me as long as I start with a .emacs that
 > turns off the menus. I can't work out what it is that's causing the
 > problem, and even the large rewrite of some of the menu code we did on
 > the master branch makes no difference. It's quite annoying.)

I'll try turning menus off the next time.  But the number of warnings
when building has by now exceeded any reasonable limit.  I can no more
see the wood for the trees

Thanks, martin

bug#45620: 28.0.50; Child frames should have their own border width and colour

[Alan Third]

On Thu, Jan 28, 2021 at 10:42:18AM +0100, martin rudalics wrote:
> > Is there some simple test I can try? I had a quick look through the
> > thread but didn't see anything.
> 
> Please with emacs -Q define
> 
> (defun my-make-child-frame ()
>   (interactive)
>   (make-frame
>    `((parent-frame . ,(selected-frame))
>      (undecorated . t)
>      (left . 0.5)
>      (top . 0.5)
>      (width . 0.3)
>      (height . 0.3)
>      (internal-border-width . 3))))
> 
> then do
> 
> (setq frame (my-make-child-frame))
> 
> and finally perform the following two experiments:
> 
> (1) Customize the background of the faces 'internal-border' and
> 'child-frame-border'.  Only customizing the latter should affect the
> child frame.
> 
> (2) Do
> 
> (set-frame-parameter frame 'internal-border-width 7)
> (set-frame-parameter frame 'child-frame-border-width 1)
> 
> Either of them should change the border width of the child frame as
> indicated.  If you now do
> 
> (set-frame-parameter nil 'internal-border-width 5)
> 
> with your normal frame selected, that frame's internal border width
> should change but the child frame's border width should remain
> unaltered.

That all works as I expect on macOS, so I guess we can say it's fine?

> > (GNUstep builds work for me as long as I start with a .emacs that
> > turns off the menus. I can't work out what it is that's causing the
> > problem, and even the large rewrite of some of the menu code we did on
> > the master branch makes no difference. It's quite annoying.)
> 
> I'll try turning menus off the next time.  But the number of warnings
> when building has by now exceeded any reasonable limit.  I can no more
> see the wood for the trees

Ah yes, I'd forgotten about that. As I recall the errors are mostly
from the GNUstep headers, so I don't know if they're caused by some
incompatibility between recent versions of GCC and GNUstep, or if
we're setting some compiler flag that GNUstep dislikes.

I don't get these warnings on my old Debian Jessie build environment,
just on the newer Buster one, but Jessie's really old by now.
-- 
Alan Third

bug#45620: 28.0.50; Child frames should have their own border width and colour

[martin rudalics]

 > That all works as I expect on macOS, so I guess we can say it's fine?

I guess so too.  Many thanks for checking.

 > I don't get these warnings on my old Debian Jessie build environment,
 > just on the newer Buster one, but Jessie's really old by now.

Here (with Buster) I especially get some 2000 lines of

2021-01-29 08:45:54.042 temacs[3778:3778] autorelease called without pool for object (0xd6f990) of class GSCInlineString in thread <NSThread: 0xbde000>{name = (null), num = 3778}

I've never seen before.

martin