Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace explicit use of Arial with default font #668

Merged
merged 2 commits into from
Oct 19, 2017
Merged

Conversation

wjwwood
Copy link
Contributor

@wjwwood wjwwood commented Oct 19, 2017

Description

This pull request replaces uses of "Arial" with the default application font from Qt with QGuiApplication::font().

Checklist

  • Required: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Optional: Created tests, which fail without this PR reference
  • Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)
    • I think this should be cherry-picked to indigo as well, but that's up to the maintainers.

@wjwwood
Copy link
Contributor Author

wjwwood commented Oct 19, 2017

Thanks @rhaschke for the fixup. 👍

@rhaschke
Copy link
Contributor

@wjwwood Thanks for your search-and-replace work in first place ;-)

@rhaschke rhaschke merged commit 9f61278 into kinetic-devel Oct 19, 2017
@rhaschke rhaschke deleted the remove_arial branch October 19, 2017 22:15
@rhaschke
Copy link
Contributor

@v4hn Should we cherry-pick this into Indigo too?

@v4hn
Copy link
Contributor

v4hn commented Oct 20, 2017

I don't believe this is desirable, unless users asks for it.

Thanks for taking care of this both of you!

@gavanderhoorn
Copy link
Member

Does Indigo RViz not have the same problem? From the comment by @wjwwood I get the impression it does?

@v4hn
Copy link
Contributor

v4hn commented Oct 20, 2017

Does Indigo RViz not have the same problem?

O.o I wasn't aware they changed the font in indigo too, until I just verified it.
That is a harsh change for a three year old release... Talking about "stable" releases.

In that case it makes sense to backport, yes. I'll add a request.

v4hn pushed a commit to v4hn/moveit that referenced this pull request Oct 20, 2017
@gavanderhoorn
Copy link
Member

@v4hn wrote:

That is a harsh change for a three year old release... Talking about "stable" releases.

it was a licensing issue. I guess it makes sense to address those, even in stable/old releases.

@wjwwood
Copy link
Contributor Author

wjwwood commented Oct 20, 2017

Does Indigo RViz not have the same problem? From the comment by @wjwwood I get the impression it does?

This pr is just "good to do", it wasn't fixing any problem. So it doesn't have to be backported to fix a problem unless you notice one in indigo. I think all the problems people were running into were due to mixing versions of rviz in strange ways in their workspaces.

That is a harsh change for a three year old release... Talking about "stable" releases.

it was a licensing issue. I guess it makes sense to address those, even in stable/old releases.

Exactly. Since you guys are only referring to it here and not distributing it then you do not have to backport to indigo like rviz did.

@gavanderhoorn
Copy link
Member

@wjwwood wrote:

edited the previous comment to add an important "do not".. :)

yes, I was getting confused ;)

@v4hn
Copy link
Contributor

v4hn commented Oct 20, 2017

Exactly. Since you guys are only referring to it here and not distributing it then you do not have to backport to indigo like rviz did.

Well,

  1. I didn't dig deep enough but can we dismiss the possibility that the Arial font used on some system was - at least in some usages - the one provided by rviz?
  2. If you removed arial usage throughout RViz in indigo, MoveIt might as well follow up for consistancy.

@wjwwood
Copy link
Contributor Author

wjwwood commented Oct 20, 2017

Yeah, I'm not sure how referring to Arial as the font family on Ubuntu does not break, since removing it from rviz, but it seems to work. I guessed that Ubuntu or Qt is silently providing a fallback font in those cases for compatibility reasons. Or maybe Arial is redistributed by Ubuntu with a correct license to do so and everything. I'm not sure.

@v4hn
Copy link
Contributor

v4hn commented Oct 20, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants