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

[ENH] DomainEditor: Indicate changed variables with bold face font #3576

Merged
merged 3 commits into from
Mar 7, 2019

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Feb 2, 2019

Issue

Fixes #2673 and fixes #3563.

Description of changes

Model remembers the original data and implements FontRole to return a bold font if the line was changed.

I sincerely don't know how to reasonably test this. It's all just a game of signals.

Includes
  • Code changes
  • Tests
  • Documentation

@codecov
Copy link

codecov bot commented Feb 2, 2019

Codecov Report

Merging #3576 into master will decrease coverage by <.01%.
The diff coverage is 52.38%.

@@            Coverage Diff            @@
##           master   #3576      +/-   ##
=========================================
- Coverage   84.31%   84.3%   -0.01%     
=========================================
  Files         370     370              
  Lines       67838   67858      +20     
=========================================
+ Hits        57197   57211      +14     
- Misses      10641   10647       +6

@janezd janezd force-pushed the domaineditor-indicate-changes branch from dda2652 to 83854c8 Compare February 2, 2019 16:45
@janezd janezd changed the title DomainEditor: Indicate changed variables with bold face font [ENH] DomainEditor: Indicate changed variables with bold face font Feb 2, 2019
@lanzagar
Copy link
Contributor

lanzagar commented Feb 4, 2019

Does not work correctly for me.
After some clicking, currently it fails even just opening the File widget and changing sepal width to meta (it does not become bold), then clicking Reload makes it bold and combinations of changes with clicking Reset, Apply, Reload almost always lead to inconsistencies.
(I cleared widget settings and it still fails as early as described above, which is strange, because at first I though it was working fine, until some random clicking).

@janezd janezd changed the title [ENH] DomainEditor: Indicate changed variables with bold face font [WIP] [ENH] DomainEditor: Indicate changed variables with bold face font Feb 14, 2019
@janezd janezd force-pushed the domaineditor-indicate-changes branch 2 times, most recently from 90c9d22 to 76d6d88 Compare March 2, 2019 08:04
@janezd janezd changed the title [WIP] [ENH] DomainEditor: Indicate changed variables with bold face font [ENH] DomainEditor: Indicate changed variables with bold face font Mar 2, 2019
@janezd
Copy link
Contributor Author

janezd commented Mar 2, 2019

It's amazing that it worked for me on macOs, but indeed fails on Linux. The problem was in a list that had to be deepcopied, not just copied.

I noticed that changing a variable's position (e.g. feature -> meta) is correctly indicated with bold font, but has no effect on the output. This is an unrelated problem that already exists on master.

@janezd
Copy link
Contributor Author

janezd commented Mar 2, 2019

LOL, changing feature -> meta of course works. I was observing the output on Select columns, which just reversed any change -- because that's exactly what it does. :)

@janezd janezd force-pushed the domaineditor-indicate-changes branch from 76d6d88 to 707a449 Compare March 2, 2019 08:23
@janezd janezd force-pushed the domaineditor-indicate-changes branch from 707a449 to 768d8ad Compare March 2, 2019 10:46
@janezd janezd force-pushed the domaineditor-indicate-changes branch from 768d8ad to a0d5515 Compare March 2, 2019 11:42
@ajdapretnar
Copy link
Contributor

This works well for me after a short test. However, changes are indicated with italics not bold. I like it that way, but we could make it clear in the PR as well.

@janezd
Copy link
Contributor Author

janezd commented Mar 4, 2019

                font = QFont()
                font.setBold(True)

gives italic!? It's bold on my machine.

@ajdapretnar
Copy link
Contributor

Sorry, I was being stupid. I was looking at Edit Domain, I obviously didn't read the PR title well enough. It is indeed in bold. And it works, too. I mean the one in File widget.

@lanzagar lanzagar merged commit b93909a into biolab:master Mar 7, 2019
@markotoplak
Copy link
Member

@janezd, I particularly like that reset button. Looks cool. :)

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.

Domain Editor in File widget should have a Reset button File: Mark modifications in DomainEditor
4 participants