D23257: Allow for easier syncing of Plasma font

[David Redondo]

davidre closed this revision.

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> filipf wrote in sddmkcm.cpp:243
> Unfortunately not yet because we still need the `kdeglobals` file for color 
> schemes. From my previous testing it's not enough to just copy some line 
> `colorScheme=Adwaita`, we actually need to copy all of the color scheme 
> values written in `kdeglobals`. IIRC that was also a bit tricky just like 
> recreating the `fonts.conf` XML file, but it may be possible to reuse 
> existing KDE color scheme functions instead of copy-pasting from config files.

Alright

BRANCH
  arcpatch-D23257

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[Filip Fila]

filipf added inline comments.

INLINE COMMENTS

> davidre wrote in sddmkcm.cpp:243
> Should I remove this then?

Unfortunately not yet because we still need the `kdeglobals` file for color 
schemes. From my previous testing it's not enough to just copy some line 
`colorScheme=Adwaita`, we actually need to copy all of the color scheme values 
written in `kdeglobals`. IIRC that was also a bit tricky just like recreating 
the `fonts.conf` XML file, but it may be possible to reuse existing KDE color 
scheme functions instead of copy-pasting from config files.

BRANCH
  arcpatch-D23257

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> sddmkcm.cpp:243
>  
>  if (!kdeglobalsPath.isEmpty()) {
>  args[QStringLiteral("kdeglobals")] = kdeglobalsPath;

Should I remove this then?

BRANCH
  arcpatch-D23257

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[Filip Fila]

filipf added a comment.


  In D23257#677396 , @davidre wrote:
  
  > I rebased it because of the rewrite that happened, can you check if 
everything is still ok @filipf
  
  
  LGTM. Instead of copying the user's entire `kdeglobals` file (which could 
also be security risk), the sync function now only reads the font value from 
`kdeglobals` and writes it to the `kde_settings.conf` file. Thanks!

BRANCH
  arcpatch-D23257

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[David Redondo]

davidre added a comment.


  I rebased it because of the rewrite that happened, can you check if 
everything is still ok @filipf

BRANCH
  arcpatch-D23257

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[David Redondo]

davidre updated this revision to Diff 83430.
davidre added a comment.


  Rebase

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23257?vs=64999=83430

BRANCH
  arcpatch-D23257

REVISION DETAIL
  https://phabricator.kde.org/D23257

AFFECTED FILES
  src/sddmkcm.cpp

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[Filip Fila]

filipf added a comment.


  I think it should work same as when I submitted the patch. It would be good 
to add a comment though that this feature is only supported for for SDDM 0.19 
and above.
  
  In D23257#514708 , @filipf wrote:
  
  > In D23257#514619 , @ngraham 
wrote:
  >
  > > Does anything bad happen if the users uses this with a version of SDDM 
that doesn't support it yet, or does the new item in the config file just get 
ignored?
  >
  >
  > Still runs for me and I'm not getting any errors when running the greeter 
in test mode, but I can't be 100% sure it's not causing any issues.
  
  
  And per this comment nothing bad will happen for versions before 0.19, in 
case there is some conservative distro out there.
  
  ///
  
  @davidre, seeing as this was one of the goals, could you also help me finish 
T12710 ? D27461 
 should likewise just work as is, I just 
didn't get any +1 on it back then.
  Task no.2 was something I couldn't accomplish though. If you have the time 
and some insight on how to do it, let me know in the task.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  easier-font-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[Nathaniel Graham]

ngraham added a comment.


  Let's do it.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  easier-font-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[David Redondo]

davidre added a comment.


  Everybody ok if I push this, before it is forgotten again?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  easier-font-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: davidre, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D23257: Allow for easier syncing of Plasma font

[David Edmundson]

davidedmundson added a comment.


  Makes sense

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  easier-font-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D23257: Allow for easier syncing of Plasma font

[Filip Fila]

filipf added a comment.


  So the SDDM patch has been merged, but I don't know when the next SDDM 
release will be. I would keep this on hold until this happens, just to be 
safe... does that make sense?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  easier-font-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D23257: Allow for easier syncing of Plasma font

[Filip Fila]

filipf updated this revision to Diff 64999.
filipf added a comment.


  Rebase on master

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23257?vs=64021=64999

BRANCH
  easier-font-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D23257

AFFECTED FILES
  src/advancedconfig.cpp

To: filipf, ngraham, #plasma, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23257: Allow for easier syncing of Plasma font

[Filip Fila]

filipf added a comment.


  In D23257#514619 , @ngraham wrote:
  
  > Does anything bad happen if the users uses this with a version of SDDM that 
doesn't support it yet, or does the new item in the config file just get 
ignored?
  
  
  Still runs for me and I'm not getting any errors when running the greeter in 
test mode, but I can't be 100% sure it's not causing any issues.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23257: Allow for easier syncing of Plasma font

[Nathaniel Graham]

ngraham added a comment.


  Does anything bad happen if the users uses this with a version of SDDM that 
doesn't support it yet, or does the new item in the config file just get 
ignored?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23257: Allow for easier syncing of Plasma font

[Filip Fila]

filipf added a comment.


  In D23257#514320 , @davidedmundson 
wrote:
  
  > Either SDDM code should deserialize the font (so that the italics and sizes 
and such work)
  
  
  This is much better than just setting the font family, I pushed a change in 
the SDDM pull request to support this now.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23257: Allow for easier syncing of Plasma font

[David Edmundson]

davidedmundson added a comment.


  We have a mixture of two things.
  
  Here we have a serialised QFont
  
  stored as
  font=Lato,11,-1,5,50,0,0,0,0,0,Regular
  
  using these magic methods:
  QDataStream ::operator<<(QDataStream , const QFont )
  
  That tell other code how to turn a QFont object into a string and back. 
Including family, italics, size, etc.
  
  In the SDDM PR we load that big text as a string, which won't go via that.
  
  We're then using this constructor
  QFont(const QString , int pointSize = -1, int weight = -1, bool italic 
= false)
  
  ---
  
  Either SDDM code should deserialize the font (so that the italics and sizes 
and such work)
  Or this code should only write the family name.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D23257

To: filipf, ngraham, #plasma, davidedmundson
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23257: Allow for easier syncing of Plasma font

[Filip Fila]

filipf created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
filipf requested review of this revision.

REVISION SUMMARY
  Currently the user needs to have a fonts replacment file in /.config/conf.d/ 
if they want to sync their Plasma font with SDDM.
  
  That's not a good solution because users simply don't have this file or 
wouldn't know how to create it themselves.
  
  In conjuction with https://github.com/sddm/sddm/pull/1191 this patch removes 
the need for additional user effort and allows for easy font syncing by writing 
the Plasma font value to the SDDM config file.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  easier-font-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D23257

AFFECTED FILES
  src/advanceconfig.cpp

To: filipf
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart