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

highlight mode for histogram or graph objects #1839

Merged
merged 4 commits into from May 15, 2018

Conversation

musinsky
Copy link
Contributor

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@couet couet self-assigned this Apr 10, 2018
@couet
Copy link
Member

couet commented Apr 11, 2018

Original JIRA request: https://sft.its.cern.ch/jira/browse/ROOT-6792

@couet
Copy link
Member

couet commented Apr 19, 2018

Hello Jan. I think what is missing is the motivation/documentation of this new functionally. May be it can go in the THistPainter and TGraphPainter doc ? right now it is not easy to understand how to use it.

You can just update the files and push the modified version in the branch PR and it will appear here.

@musinsky
Copy link
Contributor Author

Hello Olivier,
yes, you are right, this commit is without documentation. I will try to prepare documentation from this README.md to THistPainter/TGraphPainter files.

@couet
Copy link
Member

couet commented Apr 19, 2018

Yes the best would be to add a section in: https://root.cern/doc/master/classTHistPainter.html
Look how it is done in THistPainter.cxx header.

@musinsky
Copy link
Contributor Author

@couet
Copy link
Member

couet commented Apr 24, 2018

Hello Jan. Thanks for the doc but the html files are not what I am looking for. The doc is directly in THistPainter.cxx .. the html files are generated from them. Have the look at the header of THistPainter.cxx

Otherwise the doc you added looks very good. :-)

Copy link
Member

@couet couet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc is very nice but we need the .cxx file not the html.

@musinsky
Copy link
Contributor Author

musinsky commented Apr 24, 2018

Hello Olivier, the doc was generated from sources files (THistPainter.cxx and TGraphPainter.cxx files) with doxygen. Sources files with doc was added to this PR (commit 5083799), sorry for confusing comment.

@couet
Copy link
Member

couet commented Apr 24, 2018

Oh i see ! .. sorry that was my confusion ...

Copy link
Contributor Author

@musinsky musinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarified

@couet
Copy link
Member

couet commented Apr 24, 2018

Hello Jan. Sounds good and ready to be committed. We are now in the process of making ROOT 6.14
I think it is better if we commit your changes just after we do this new release. That will put less stress on us to polish this new feature.

@musinsky
Copy link
Contributor Author

OK, after ROOT new release.

@couet
Copy link
Member

couet commented Apr 24, 2018

Thanks for your patience :-)

@couet
Copy link
Member

couet commented Apr 24, 2018

The first animated gif you added in http://alice.saske.sk/rootdoc/html/classTHistPainter.html#HP30 is a bit wide which forces the web bowser to adjust the display width, which later may creates weird layout of the subsequent pages one will browse . Would it be possible to have this animation such it does not force the web browser to adjust the display width? may be by putting the two canvas on top of each other and not side by side ?

@musinsky
Copy link
Contributor Author

I can create a new animation (this is not a problem), which width is optimal (maximum) ?
Original gif has size 1200x530, Half size gif has 600x265.
Create animated gif with a width of around 800-900 ?

@couet
Copy link
Member

couet commented Apr 24, 2018

I think that keeping the same size for the canvases but drawing them on top of each other should be fine.

@musinsky
Copy link
Contributor Author

I have tried to make with two canvases (two gifs), but problem with "synchronization" in browser (gif1 "start animating" in different time as gif2, see on this temporary site), must be as one animated gif.

I think there are only two possibilities:

  • create a new animation with width for example 900
  • or set width in source \image html hlHisto3.gif "Highlight mode" width=900

@couet
Copy link
Member

couet commented Apr 24, 2018

Why don't you simply make one animated gif with the two canvases on top of each other ? it should be possible ? The tool you are using to make them does not allow this ?

@musinsky
Copy link
Contributor Author

Now I've understood, sorry ... will be done, no problem

@musinsky
Copy link
Contributor Author

Done (and regenerated html doc with doxygen)
http://alice.saske.sk/rootdoc/html/classTHistPainter.html#HP30

@couet
Copy link
Member

couet commented Apr 25, 2018

much better ! Thanks !

@couet couet merged commit e4d37e8 into root-project:master May 15, 2018
@musinsky musinsky deleted the highlight branch June 22, 2018 13:06
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.

None yet

3 participants